Remove last few report.managerEmail uses#30515
Conversation
|
Nice! Should we have a C+ test it? |
|
@puneetlath Yesssss definitely! 👍 I'm going to do a bit of testing on my side too, then will look for a C+ to take this on 🙏 |
|
Sweet, sounds good! |
|
FINALLY tests pass, let's get some C+ to help test now! I uploaded a vid of some tests w/ ios & web |
|
Thanks @shubham1206agra for volunteering to help with this! |
|
@Beamanator Can you merge latest main here? |
|
@shubham1206agra done! 👍 |
|
@shubham1206agra just checking, are you able to review this today? 🙏 |
|
@Beamanator Can you review this video (especially at the last)? Screen.Recording.2023-10-31.at.8.14.46.PM.mp4 |
|
@shubham1206agra sorry I didn't see your message! Is there anything in particular you think isn't working properly? I'll watch and see if I notice anything |
|
Aah it looks like deleting tasks isn't updating the other user live - have you noticed this before @thienlnam ? (see video above, around minute 2:22) - it looks like it did update both users when the user who created the task deleted it (around minute 1:22), but not when the other person deletes it (minute 2:22) I guess the question @shubham1206agra is: Does that bug exist in staging / prod as well? If so, we can move forward with this PR. If not, it could be caused by my changes |
|
@Beamanator I think you should first repro this, and test the same in production or staging |
|
@shubham1206agra it would be great if you could try, if you can't repro in staging/prod I'm happy to give it a go - but I'm pretty busy with other things at the moment |
|
@Beamanator The Same can be repro in prod. I think we can continue the review now. |
|
Nice, yeah I agree @shubham1206agra 👍 |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2023-10-31.at.8.14.46.PM.mp4MacOS: Desktop |
|
@marcaaron 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] |
|
🎯 @shubham1206agra, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #30695. |
|
Yeah there are currently some PRs to fix some functionality with cancel task |
|
Good to know @thienlnam 🙏 I'll fix merge conflicts 😓 |
|
Conflicts fixed! @shubham1206agra can you please test again before we move forward? 🙏 |
|
Not sure why Melvin requested a review from me. Looks like an internal engineer was already assigned. Gonna unsubscribe, but lmk if an additional review is needed. |
|
@Beamanator Still works fine |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/Gonals in version: 1.3.96-0 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.96-15 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.97-7 🚀
|
cc @thienlnam since hopefully this will be the last change needed for tasks 🙏 (related to email -> accountID migration)
also cc @puneetlath so you're aware! we're getting close!
Details
We're working on getting rid of emails & using accountIDs instead - see https://github.com/Expensify/Expensify/issues/294647
Purpose = to increase privacy / prevent emails from being leaked around the app - in onyx & in API calls
Note: We need to watch out for creating reports / tasks with BRAND NEW users that we may / may not have personal details for yet. I believe we now store email addresses in personal details when we start chats (since we must have typed in their email address?) but I'm not 100% sure, which is why it needs to be tested.
Fixed Issues
$ Related to https://github.com/Expensify/Expensify/issues/294647
$ Finishes up https://github.com/Expensify/Expensify/issues/299956
Tests
Make sure task actions still work well:
Note: You should test on multiple devices so you can make sure all task updates work correctly for:
Similarly, make a few money requests & edit / delete them, make sure everything still works perfectly
Offline tests
Same as above, everything should work optimistically offline
QA Steps
Same as above
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
Android: Native
Android: mWeb Chrome
iOS: Native
(see MacOS: Chrome / Safari video)
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2023-10-27.at.5.44.39.PM.mov
MacOS: Desktop