-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Use parentReportID for report edits when modifying thread parent action #20945
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
❗ 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 📖. |
|
@Julesssss 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] |
Julesssss
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing well for me 👍 (ignoring the duped onyxUpdate that is mentioned)
Reviewer Checklist
Screenshots/VideosWebweb.movMobile Web - ChromeMobile Web - Safarimsafari.movDesktopdesktop.moviOSios.movAndroid |
0xmiroslav
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests well for me too! even when offline
|
✋ 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/Julesssss in version: 1.3.30-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.30-5 🚀
|
|
@NikkiWines can you please create payment GH for me? |
|
I reached out to someone on our BZ team to get that sorted for you @0xmiroslav, sorry for the delay!! |
|
@NikkiWines can you confirm payment amount in #25809 then I'll take care of via an Upwork job? Thx |
Details
When a user creates a thread and edits the
parentActionof that thread, we need to send theparentReportID, not thechildReportIDin the resulting network request. We know the user is editing theparentActionwhen theparentReportActionIDandreportActionIDare the same.cc: @chiragsalian
Fixed Issues
Partially resolves https://github.com/Expensify/Expensify/issues/282991
PROPOSAL: N/A
Tests
Non-threaded comment
NetworktabNetworktab, confirm theonyxDatain the response for theUpdateCommentcommand has anonyxMethodmergewith the keyreportActions_<reportID>reportIDmatches thereportIDof the report where the comment was edited.Threaded non-parent action comment
NetworktabNetworktab, confirm theonyxDatain the response for theUpdateCommentcommand has anonyxMethodmergewith the keyreportActions_<reportID>reportIDmatches thereportIDof the thread report where the comment was edited.Threaded parent action comment
NetworktabNetworktab, confirm theonyxDatain the response for theUpdateCommentcommand has anonyxMethodmergewith the keyreportActions_<reportID>reportIDmatches thereportIDof the parent report where the thread was initiated.reportActions_<reportID>objects in this case, this is resolved in a separate internal PROffline tests
N/A
QA Steps
Same Test steps, Web QA only
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
N/A no UI changes.
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android