[No QA] Report_AddComment Refactor Part 1 #9328
Conversation
marcaaron
changed the title
Use timestamp based solution to determine if we need to send a notification set notificationPreference in report object Use notification preference from Pusher for now improve comment Get rid of sendLocalNotification and just perform directly in Onyx.connect change name to viewNewReportAction Make a better diff Fix tests
marcaaron
force-pushed
the
marcaaron-localNotifications
branch
from
d1fcde6
to
0b0616d
Compare
marcaaron
changed the title
|
I tried to add in the *By not working, I mean it causes extreme lag and triggers many browser notifications. |
| unreadActionCount: newMaxSequenceNumber - (lastReadSequenceNumbers[reportID] || 0) - ReportActions.getDeletedCommentsCount(reportID, lastReadSequenceNumbers[reportID] || 0), | ||
| }; | ||
|
|
||
| Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, updatedReportObject); |
There was a problem hiding this comment.
Thinking from an offline perspective. Let's say I'm offline and eventually I receive the new report action. If the 10-second check returns early we won't run this code and thus won't update the unreadActionCount, right?
There was a problem hiding this comment.
If you are offline then you can't get any Pusher updates at all. There is no "eventually" - you missed it and it's never coming.
There was a problem hiding this comment.
If you're asking what happens when we come back online - then we would fetch the reports which should update the unreadActionCount for any reports. If you manage to miss a report action update because you're offline and then come back online before the 10 second window - I guess you'd see a notification - which seems alright.
There was a problem hiding this comment.
Ah okay, thanks for clarifying!
| Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, updatedReportObject); | ||
|
|
||
| // If chat report receives an action with IOU and we have an IOUReportID, update IOU object | ||
| if (action.actionName === CONST.REPORT.ACTIONS.TYPE.IOU && action.originalMessage.IOUReportID) { |
There was a problem hiding this comment.
Similar comment for this logic as for above.
There was a problem hiding this comment.
I think the answer is the same here. If you miss this update then we just need to have the "reconnection" callbacks account for this data. All the IOU reports should ideally be sent whenever we are "reconnecting".
|
✋ 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 @yuwenmemon in version: 1.1.74-0 🚀
|
|
🚀 Deployed to production by @yuwenmemon in version: 1.1.74-2 🚀
|
| // If we are currently viewing this report do not show a notification. | ||
| if (reportID === lastViewedReportID && Visibility.isVisible()) { | ||
| Log.info('[LOCAL_NOTIFICATION] No notification because it was a comment for the current report'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
We didn't consider whether the window has focus, which led to a regression here
Details
Was looking into refactoring
Report_AddComment, but quickly realized it's kind of a mess and thought it would be helpful to split things up into:My idea here is to basically take
updateReportWithNewAction()and minimize it so that it is only used to set data that the server can set. This will be the basis for the next refactoring step - which is to move that stuff intoWeb-Expensifyand then we'll follow that up with the last step to see what else we can do to move more logic into the backend.Fixed Issues (Related To)
https://github.com/Expensify/Expensify/issues/211241
Tests
PR Review Checklist
PR Reviewer Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followed/** comment above it */displayNamepropertythisproperly 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)QA Steps