Fix user are able to flag their own comment #21771
Conversation
|
@mananjadhav 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] |
| @@ -61,6 +67,11 @@ function getReportID(route) { | |||
|
|
|||
| function FlagCommentPage(props) { | |||
| let reportAction = props.reportActions[`${props.route.params.reportActionID.toString()}`]; | |||
|
|
|||
| // Handle threads if needed | |||
| if (reportAction === undefined || reportAction.reportActionID === undefined) { | |||
There was a problem hiding this comment.
Would it make any difference if we move this if block to shouldShowFlagComment ?
There was a problem hiding this comment.
we should get the correct reportAction here to pass it into Report.flagComment.
| @@ -138,10 +148,15 @@ function FlagCommentPage(props) { | |||
| /> | |||
| )); | |||
|
|
|||
| const shouldShowLoading = props.isLoadingReportData || props.report.isLoadingReportActions; | |||
There was a problem hiding this comment.
I am a bit concerned that we're using props.report directly without confirming if it is undefined? I think it makes sense to add. the following:
if (isEmpty(props.report)) {
Navigation.dismissModal();
return;
}
There was a problem hiding this comment.
I don't think so. We defined defaultProps for report is an empty object and that work as well.
There was a problem hiding this comment.
I was concerned about non-existent report, but I tested it shows up the invalid access page. Thanks for the patience here.
Co-authored-by: Manan <manan.jadhav@gmail.com>
Reviewer Checklist
Screenshots/VideosWeb
Using Secondary Login Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android
|
|
@tylerkaraszewski Ready for your review, I've tested and finished the checklist. 🎀 👀 🎀 C+ Reviewed. |
|
✋ 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/tylerkaraszewski in version: 1.3.35-0 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 1.3.35-5 🚀
|
1 similar comment
|
🚀 Deployed to production by https://github.com/Julesssss in version: 1.3.35-5 🚀
|
| if (shouldShowLoading) { | ||
| return <FullscreenLoadingIndicator />; | ||
| } |
There was a problem hiding this comment.
@dukenv0307 Why this code is needed? I couldn’t find any reference and it seems to be causing a regression in #23125
There was a problem hiding this comment.
@fedirjh We need it to wait the report data and report action data are loaded completely before we should not found page or flag page.
There was a problem hiding this comment.
If we remove this code, when we open the page by deep link or reload the page we will see page not found will display briefly before flag page is displayed.
There was a problem hiding this comment.
We need it to wait the report data and report action data are loaded completely before we should not found page or flag page.
This approach will not work in offline mode. Instead, we should check if report is empty.
There was a problem hiding this comment.
I think updating the check to this will be fine
const shouldShowLoading = (props.isLoadingReportData && _.isEmpty(props.report) || (_.isEmpty(props.reportActions) && props.report.isLoadingReportActions)
There was a problem hiding this comment.
This approach will not work in offline mode. Instead, we should check if report is empty.
This will not work in offline mode when we do something to make the openReport API is called
There was a problem hiding this comment.
Let's continue the discussion on the linked issue #23125
Details
Fix senders are not able to flag their own comment
Fixed Issues
$ #21212
PROPOSAL: #21212 (comment)
Tests
Offline tests
Same as above
QA Steps
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
Screencast.from.28-06-2023.10.50.01.webm
Mobile Web - Chrome
Record_2023-06-28-10-58-45.mp4
Mobile Web - Safari
21212-safari.mp4
Desktop
desktop.mp4
iOS
ios.1.mp4
Android
21212.mp4