Show info about workspace members added by secondary logins#23702
Show info about workspace members added by secondary logins#23702neil-marcellini merged 25 commits intomainfrom
Conversation
|
@shawnborton would you please take a look at the web screenshot I included and the code if you like, and give me some feedback on the design? |
|
I think this feels good to me! |
|
Ok this is ready for review now, except that I need the backend PR to be live before it can be tested by C+. I'm putting the backend PR up for review now. |
|
@allroundexperts Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-10-24.at.6.56.15.AM.movMobile Web - ChromeScreen.Recording.2023-10-24.at.7.37.20.AM.movMobile Web - SafariScreen.Recording.2023-10-24.at.7.28.39.AM.movDesktopScreen.Recording.2023-10-24.at.7.46.26.AM.moviOSScreen.Recording.2023-10-24.at.7.33.18.AM.movAndroidScreen.Recording.2023-10-24.at.7.40.14.AM.mov |
|
Hi @neil-marcellini, can you please resolve the conflicts and address the comments? |
|
Sorry for the delay I've been focusing on distance requests. I'm also going on vacation for the rest of the week. I'll try to squeeze this in today but I might have to get to it much later. |
|
Woah I forgot about this for a long time, sorry. I'm going to make it a draft until I get it updated. |
|
I have a fix for the bug in progress, but I didn't quite get it working |
That's resolved and this is ready for another review! |
|
@MonilBhavsar Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
MonilBhavsar
left a comment
There was a problem hiding this comment.
Looks good overall. Couple of minor NAB comments
|
I'm going to merge now because this PR / issue has been open for months. I can fix the NABs in a follow up. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
|
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 1.3.93-0 🚀
|
Ok I kept my word, here is the clean up! [No QA] Clean up for added by secondary login messages |
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.93-1 🚀
|
|
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 1.3.94-0 🚀
|
1 similar comment
|
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 1.3.94-0 🚀
|
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.94-2 🚀
|
Details
Previously, if you added a workspace member with their primary login they wouldn't show up and a message saying "Member not found" would appear. If you refreshed than the member would show up under their primary login, because that's how users are added to workspaces / policies. It's a confusing flow.
Now the member will be added with their secondary login optimistically, then get added by their primary (preferred) login. We'll show an informative message "Some users were added with their primary logins" and under each primary login added with a secondary login it will say "Added by secondary login neil+secondary@expensifail.com", for example. When the main error message is dismissed the informative messages will disappear, or if you sign out. The messages are meant to be temporary.
I also created a new "MessagesRow" component which makes it easy to show dismissible info or error messages. It's extracted from OfflineWithFeedback and results in the same behavior.
Fixed Issues
$ #21639
PROPOSAL: N/A
Tests
Set up
Test main flow
Test removing all members invited by secondary login
Offline tests
Repeat the same while offline before inviting, then go online and verify.
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
I only tested on web and iOS because the changes should be platform independent and C+ will test all platforms.
Web
Main flow
web.mov
Remove members
removeMembersWeb.mov
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
iOS.MP4
Android