Skip to content

Sync workspace member across devices after removing member(s)#7460

Merged
Luke9389 merged 5 commits into
Expensify:mainfrom
K4tsuki:remove_member__
Feb 4, 2022
Merged

Sync workspace member across devices after removing member(s)#7460
Luke9389 merged 5 commits into
Expensify:mainfrom
K4tsuki:remove_member__

Conversation

@K4tsuki

@K4tsuki K4tsuki commented Jan 31, 2022

Copy link
Copy Markdown
Contributor

Details

Sync workspace member across device after member(s) removed.

Fixed Issues

$ #7095

Tests

  • Verify that no errors appear in the JS console
  1. Open workspace member page with same account on two device
  2. Remove member(s) on a device and refresh member page in other device
  3. Members of workspace must be same on those two device
  4. In android must go back to setting page and then go to member page to update the member list.

QA Steps

  • Verify that no errors appear in the JS console
  1. Open workspace member page with same account on two device
  2. Remove member(s) on a device and refresh member page in other device
  3. Members of workspace must be same on those two device
  4. In android must go back to setting page and then go to member page to update the member list.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

remove_web.mp4

Mobile Web

remove_c_mweb.mp4

Desktop

iOS

Android

android_remove.mp4

@MelvinBot MelvinBot requested review from Luke9389 and parasharrajat and removed request for a team January 31, 2022 02:07
Comment thread src/libs/actions/Policy.js Outdated
K4tsuki and others added 2 commits February 1, 2022 07:55
Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>
@K4tsuki

K4tsuki commented Feb 1, 2022

Copy link
Copy Markdown
Contributor Author

Fixing lint error

parasharrajat
parasharrajat previously approved these changes Feb 1, 2022

@parasharrajat parasharrajat left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works fine for the linked issue.

There could be unintended side-effects of this change. withFullPolicy is used on all workspace pages. we use Onyx.merge to merge the changes but for this we are using Onyx.set. There are limitations of using Onyx.set. It cancels the merge requests which could interfere with other Workspace pages operations. This is an assumption so It is possible that it never happens.

But Onyx poses a challenge for us and it does not give us any other choice to fix the linked issue ATM. So I am approving this.

cc: @Luke9389

🎀 👀 🎀 C+ reviewed

@K4tsuki

K4tsuki commented Feb 3, 2022

Copy link
Copy Markdown
Contributor Author

cc: @Luke9389 All checks have passed

@Luke9389

Luke9389 commented Feb 3, 2022

Copy link
Copy Markdown
Contributor

There could be unintended side-effects of this change.

@parasharrajat, this concerns me. Before approving I'm going to go read up on Onyx and be sure this change is exactly what we want it to be.

@K4tsuki

K4tsuki commented Feb 3, 2022

Copy link
Copy Markdown
Contributor Author

@Luke9389 We have been using Onyx.set in removeMember and there was no problem because of this.

Onyx.set(key, policyDataWithMembersRemoved);

Also Kidroca tell to us to using Onyx.set in this issue in slack conversation.

I have tested it on navigating between workspaces, add and remove members and there is no problem.

@Luke9389 Luke9389 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there's now a merge conflict. Please merge main

@K4tsuki

K4tsuki commented Feb 4, 2022

Copy link
Copy Markdown
Contributor Author

@Luke9389 main merged.

@Luke9389 Luke9389 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reviewing some materials, I think this is fine.

@Luke9389 Luke9389 merged commit 45d488d into Expensify:main Feb 4, 2022
@OSBotify

OSBotify commented Feb 4, 2022

Copy link
Copy Markdown
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify

OSBotify commented Feb 7, 2022

Copy link
Copy Markdown
Contributor

🚀 Deployed to staging by @Luke9389 in version: 1.1.37-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify

OSBotify commented Feb 9, 2022

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.37-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants