Add code to NewDot to handle both new and old Pusher event types#18192
Conversation
|
@eVoloshchak @grgia One of you needs to 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] |
| PREFERRED_LOCALE: 'preferredLocale', | ||
| EXPENSIFY_CARD_UPDATE: 'expensifyCardUpdate', | ||
| SCREEN_SHARE_REQUEST: 'screenshareRequest', |
There was a problem hiding this comment.
What's the migration plan? We start sending both old and new events from the backend, then remove the old one when this is released?
There was a problem hiding this comment.
No, it's more simple than that. Web-E just needed to be updated to send these three events as Onyx updates, which NewDot already supports. Once that Web-E change is in production, then the HOLD can be removed on this.
There was a problem hiding this comment.
I am confused... if web-e stopped sending the old events, then all existing apps will be broken till this is deployed, no??
There was a problem hiding this comment.
No. Older versions of NewDot (for like the past several months) can all handle the onyx update event. Web-E is just switching over to using that.
| }); | ||
|
|
||
| // Live-update an user's preferred locale | ||
| Pusher.subscribe(pusherChannelName, Pusher.TYPE.PREFERRED_LOCALE, (pushJSON) => { |
There was a problem hiding this comment.
I assume this will be migrated in the web-e code?
There was a problem hiding this comment.
Yep, it was already migrated. In fact, Web-E had stopped using this event in favor of an Onyx update event a while ago (during the big API refactoring) and this was never properly cleaned up.
|
@eVoloshchak @grgia This PR is unheld and ready for review |
|
@eVoloshchak will you be able to get a PR checklist going for this? It should be very quick testing. |
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-05-09.at.22.11.46.movMobile Web - ChromeScreen_Recording_20230509-221034_Chrome.mp4Mobile Web - SafariScreen.Recording.2023-05-09.at.22.08.28.movDesktopScreen.Recording.2023-05-09.at.22.11.22.moviOSScreen.Recording.2023-05-09.at.22.07.58.movAndroidScreen_Recording_20230509-220938_New.Expensify.mp4 |
eVoloshchak
left a comment
There was a problem hiding this comment.
Looks good, works normal
|
🎯 @eVoloshchak, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #18668. |
|
OK, I fixed a conflict so this is ready to be merged if you want to do the honors @iwiznia |
|
✋ 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/tgolen in version: 1.3.14-0 🚀
|
|
@tgolen Can you confirm server side internally? We will run regular regressions. |
|
I confirmed this and have marked this PR off the checklist. |
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.3.14-14 🚀
|
Details
This code sets up the app to be able to handle unified pusher events
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/279349
Tests
The only thing needing to be tested is that there are no regressions with the existing pusher updates. This can be done by:
Offline tests
None
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
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android