Refactor Workspace Invite Member page #5726

parasharrajat · 2021-10-08T09:44:59Z

Details

Known issues

Searching for a selected user shows a duplicate name. https://expensify.slack.com/archives/C01GTK53T8Q/p1633961442111100
Members are being removed from Onyx after adding. (Fixed).
UI is breaking on Android. (Fixed)

Fixed Issues

$ #4775
$ #5723

Tests | QA Steps

Open NewDot.
Go to any workspace from the Settings page.
navigate to Manage members.
Click the Invite button.
Select the user from the list.
Click Invite members should be added to Workspace.

Tested On

Screenshots

Web | Desktop

Mobile Web

iOS

Android

parasharrajat · 2021-10-13T07:10:49Z

Heads up: There is an issue with this PR where members are being removed from Onyx as soon as they are added. I am not able to determine the cause of it. Its driving me nuts. Help needed.

I pushed it for review to get more Eyes

src/pages/workspace/WorkspaceInvitePage.js

parasharrajat · 2021-10-15T14:05:36Z

src/libs/actions/Policy.js

@@ -248,7 +248,7 @@ function invite(logins, welcomeNote, policyID) {
    policy.alertMessage = '';

    // Optimistically add the user to the policy
-    Onyx.set(key, policy);
+    Onyx.merge(key, policy);


I had to make this change otherwise, newly added members were being removed as soon as they were added to Onyx.

I am not sure how to debug the Onyx but I tried debugging the code and somehow internal Keychanged handler is getting old value in the next tick.

parasharrajat · 2021-10-15T14:42:15Z

@Julesssss Ready for review.

kidroca · 2021-10-15T15:46:30Z

@parasharrajat
Here's what happens when Onyx.set was used

When we first click "Invite" an Onyx.merge is triggered from setWorkspaceErrors. It of course uses the policy before we've merged the invited person to it. A merge is pending
Right after the click we update the policy with one more member, this happens before the merged data is saved to storage
And now the merged data (that used the old policy) calls Onyx.set again which causes the bug

1.New.Expensify.-.Google.Chrome.2021-10-15.17-49-48.mp4

This is an issue with how Onyx.merge works and race conditions.
When it happens that both Onyx.set and Onyx.merge are used close in time and it happens that they both modify the same key, Onyx.set is the operation that will complete first and then Onyx.merge would have read an older value and write that to the key

When we always use Onyx.merge there's no such problem, because merges are queued and applied together

There's no single way to fix the race condition at the root

Onyx.set usage can be discouraged because of this
You can argue that if a set for a key happens it should abort any pending merges for that key. E.g, waiting for the merges to complete and then calling set to write a new value is the same thing but slower. Making merge wait for set to complete (when it's for the same key) is also an option but seems error prone - you can revive values that set wanted to get rid of
Merges don't have to read before writing, this is also a performance improvement but it's too big of a change

I think you've found the solution for the problem at hand - use Onyx.merge

For the future we can consider exposing only one way for updating Onyx values so to not cause race conditions and make people wonder should they use set or merge
E.g. Onyx.update(key, value, options)

kidroca · 2021-10-15T16:08:05Z

Another take on this problem is why do we store the error state for policy in Onyx? We see that it causes frequent writes to storage and often it writes the same value over and over.

Do we need the error to be persisted so we display it again later, when the user is back at this page or if they restart the app?
I know there's a push towards saving everything in Onyx and I think it's wrong - local component state has no place in persistent storage - for example we're not persisting serachValue or the composer box input for every character that changes. IMO it's ok to store a draft message as this is something you might return to, but to persist an error when we already have component state, and then make every interaction clear it by overwriting storage is like shooting our own foot.

Seeing a code like this reviews that we're not interested in a persisted value and we're using persistent storage for something temporary

 componentDidMount() {
        this.clearErrors();
    }

If we're clearing errors on mount it's obvious we don't care about errors that happened in the past. Then why use persistent storage at all. If Onyx was the only way we dealt with state then I agree that we should always use that, but many components have local state, and this one does too, and if we're not interested in persisted errors, we should just skip persisting them

parasharrajat · 2021-10-15T16:30:12Z

I agree with @kidroca's point here. But I followed the approach which was already being used in this component and other pages.

So I won't argue the approach. Open to suggestions, if any?

parasharrajat · 2021-10-15T16:31:29Z

Great Explanation. @kidroca. Thanks for investing time in this. I think we are good for this PR as merge and set have no difference in this scenario.

src/pages/workspace/WorkspaceInvitePage.js

Julesssss · 2021-10-18T17:20:28Z

Ah, so it was fixes? I've not seen this bot warning before 🤷

My guess is that it's because you have two different 'keywords': $ & fixes. Multiple keywords is usually fine IIRC.

Julesssss · 2021-10-18T17:25:59Z

Awaiting approval from Design.

parasharrajat · 2021-10-18T21:45:36Z

@shawnborton Let me know if #5726 (comment) looks good. Please.

shawnborton · 2021-10-18T22:11:43Z

Looks good, but let's remove the styling and icon from the privacy policy link and just make it blue. Thoughts on that?

Or actually crazy idea, can we just put the privacy policy link after the list of people? This way it's a bit more hidden by default?

parasharrajat · 2021-10-18T22:14:36Z

Looks good, but let's remove the styling and icon from the privacy policy link and just make it blue. Thoughts on that?

Yup. Will look better and I think Icon can stay as it is informative.

Or actually crazy idea, can we just put the privacy policy link after the list of people? This way it's a bit more hidden by default?

Where exactly? you mean at the end of the user list shown only when scrolled. I doubt that.

shawnborton · 2021-10-18T22:17:49Z

What makes you say you doubt it? We can always ask our legal team for advice here too.

parasharrajat · 2021-10-18T22:25:20Z

If the user has about 30 users. He will likely scroll to the end.
Secondly, if a user searches for a user and there is a single match, the link will be brought to the view and I don't feel good about it.

And this is complex to do ATM. 😄 .

shawnborton · 2021-10-18T22:27:06Z

Got it, that's fair feedback. In the very least, let's see it in just blue and perhaps right-aligned as well. Thanks!

parasharrajat · 2021-10-18T22:43:59Z

@shawnborton

shawnborton · 2021-10-18T22:48:03Z

Nice, can we see it without the icon as well?

parasharrajat · 2021-10-18T22:57:33Z

@shawnborton

shawnborton · 2021-10-18T22:58:35Z

Okay sweet, thanks for sharing! I think now that I see it, left-aligned might look better. Then I think it's all good on my end. Thanks Rajat!

parasharrajat · 2021-10-18T22:59:30Z

No icon? @shawnborton

shawnborton · 2021-10-18T23:01:24Z

Correct, blue and no icon.

parasharrajat · 2021-10-18T23:04:43Z

Changes are done as requested. Ready for merge.

shawnborton · 2021-10-18T23:05:47Z

Nice, I think you just need to update screenshots and then we're good there.

botify · 2021-10-18T23:07:52Z

❗ Please, do not use Github auto-linking keywords such as these: close, closes, closed, fix, fixes, fixed, resolve, resolves or resolved.

For more details, see the Contributing Guidelines, specifically Submit your pull request for a final review 📖.

botify · 2021-10-18T23:21:01Z

❗ Please, do not use Github auto-linking keywords such as these: close, closes, closed, fix, fixes, fixed, resolve, resolves or resolved.

For more details, see the Contributing Guidelines, specifically Submit your pull request for a final review 📖.

botify · 2021-10-18T23:23:33Z

❗ Please, do not use Github auto-linking keywords such as these: close, closes, closed, fix, fixes, fixed, resolve, resolves or resolved.

For more details, see the Contributing Guidelines, specifically Submit your pull request for a final review 📖.

OSBotify · 2021-10-19T09:41:46Z

✋ 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 · 2021-10-23T01:33:06Z

🚀 Deployed to staging by @Julesssss in version: 1.1.8-10 🚀

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

OSBotify · 2021-10-28T20:57:58Z

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

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

parasharrajat added 8 commits October 8, 2021 15:06

refactor Invite Member page

b7fb045

exclude existing members

1d46238

adjustments

4a8161c

manage error handling for Workspace invite page

5eb901e

Pull changes form Main

d2bf778

add loader for animation hack

668722e

Pull latest changes from main

e0d5e6b

remove focus on select

786e1d9

parasharrajat marked this pull request as ready for review October 13, 2021 07:05

parasharrajat requested a review from a team as a code owner October 13, 2021 07:05

botify requested a review from Julesssss October 13, 2021 07:05

MelvinBot requested review from Julesssss and removed request for a team and Julesssss October 13, 2021 07:05

parasharrajat changed the title ~~Refactor Workspace Invite Member page~~ [WIP] Refactor Workspace Invite Member page Oct 13, 2021

Santhosh-Sellavel reviewed Oct 13, 2021

View reviewed changes

src/pages/workspace/WorkspaceInvitePage.js Outdated Show resolved Hide resolved

parasharrajat added 4 commits October 15, 2021 05:15

fix: Hack onyx issue.

034df24

Merged latest changes

273d81f

Pull main into

739cc4e

fix: Workspace Invitepage styles

f66dd56

parasharrajat changed the title ~~[WIP] Refactor Workspace Invite Member page~~ Refactor Workspace Invite Member page Oct 15, 2021

parasharrajat commented Oct 15, 2021

View reviewed changes

parasharrajat mentioned this pull request Oct 15, 2021

[HOLD for payment 2021-11-04] Allow inviting multiple users to a workspace by reusing the OptionsSelector component #4775

Closed

Julesssss reviewed Oct 18, 2021

View reviewed changes

src/pages/workspace/WorkspaceInvitePage.js Show resolved Hide resolved

kidroca mentioned this pull request Oct 18, 2021

[Discussion|Onyx] Mixing Onyx.set and Onyx.merge usages leads to race condition #5930

Closed

Fix: flex issue

76ca556

Update link UI

add3414

Julesssss approved these changes Oct 19, 2021

View reviewed changes

Julesssss merged commit a5f2739 into Expensify:main Oct 19, 2021

parasharrajat mentioned this pull request Oct 21, 2021

Adds background to label #5959

Merged

5 tasks

OSBotify mentioned this pull request Oct 23, 2021

Deploy Checklist: New Expensify 2021-10-23 #6017

Closed

luacmartins mentioned this pull request Apr 5, 2022

Add missing copy for pleaseEnterValidLogin #8500

Merged

89 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Workspace Invite Member page #5726

Refactor Workspace Invite Member page #5726

parasharrajat commented Oct 8, 2021 •

edited by Julesssss

Loading

parasharrajat commented Oct 13, 2021 •

edited

Loading

parasharrajat Oct 15, 2021 •

edited

Loading

parasharrajat commented Oct 15, 2021

kidroca commented Oct 15, 2021

kidroca commented Oct 15, 2021

parasharrajat commented Oct 15, 2021

parasharrajat commented Oct 15, 2021

Julesssss commented Oct 18, 2021 •

edited

Loading

Julesssss commented Oct 18, 2021

parasharrajat commented Oct 18, 2021

shawnborton commented Oct 18, 2021

parasharrajat commented Oct 18, 2021

shawnborton commented Oct 18, 2021

parasharrajat commented Oct 18, 2021

shawnborton commented Oct 18, 2021

parasharrajat commented Oct 18, 2021

shawnborton commented Oct 18, 2021

parasharrajat commented Oct 18, 2021

shawnborton commented Oct 18, 2021

parasharrajat commented Oct 18, 2021 •

edited

Loading

shawnborton commented Oct 18, 2021

parasharrajat commented Oct 18, 2021

shawnborton commented Oct 18, 2021

botify commented Oct 18, 2021

botify commented Oct 18, 2021

botify commented Oct 18, 2021

OSBotify commented Oct 19, 2021

OSBotify commented Oct 23, 2021

OSBotify commented Oct 28, 2021

Refactor Workspace Invite Member page #5726

Refactor Workspace Invite Member page #5726

Conversation

parasharrajat commented Oct 8, 2021 • edited by Julesssss Loading

Details

Known issues

Fixed Issues

Tests | QA Steps

Tested On

Screenshots

Web | Desktop

Mobile Web

iOS

Android

parasharrajat commented Oct 13, 2021 • edited Loading

parasharrajat Oct 15, 2021 • edited Loading

Choose a reason for hiding this comment

parasharrajat commented Oct 15, 2021

kidroca commented Oct 15, 2021

kidroca commented Oct 15, 2021

parasharrajat commented Oct 15, 2021

parasharrajat commented Oct 15, 2021

Julesssss commented Oct 18, 2021 • edited Loading

Julesssss commented Oct 18, 2021

parasharrajat commented Oct 18, 2021

shawnborton commented Oct 18, 2021

parasharrajat commented Oct 18, 2021

shawnborton commented Oct 18, 2021

parasharrajat commented Oct 18, 2021

shawnborton commented Oct 18, 2021

parasharrajat commented Oct 18, 2021

shawnborton commented Oct 18, 2021

parasharrajat commented Oct 18, 2021

shawnborton commented Oct 18, 2021

parasharrajat commented Oct 18, 2021 • edited Loading

shawnborton commented Oct 18, 2021

parasharrajat commented Oct 18, 2021

shawnborton commented Oct 18, 2021

botify commented Oct 18, 2021

botify commented Oct 18, 2021

botify commented Oct 18, 2021

OSBotify commented Oct 19, 2021

OSBotify commented Oct 23, 2021

OSBotify commented Oct 28, 2021

parasharrajat commented Oct 8, 2021 •

edited by Julesssss

Loading

parasharrajat commented Oct 13, 2021 •

edited

Loading

parasharrajat Oct 15, 2021 •

edited

Loading

Julesssss commented Oct 18, 2021 •

edited

Loading

parasharrajat commented Oct 18, 2021 •

edited

Loading