fix: Room - Create room whisper reappears when interacting with it after workspace is deleted.#50692
Conversation
…ter workspace is deleted. Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@rayane-djouah 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] |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
| import * as ReportConnection from './ReportConnection'; | ||
| import type {OptimisticIOUReportAction, PartialReportAction} from './ReportUtils'; | ||
| import {canUserPerformWriteAction, getReport} from './ReportUtils'; | ||
| import StringUtils from './StringUtils'; |
There was a problem hiding this comment.
@rayane-djouah, I'm not sure how to fix this warning :(, please let me know if you have some any idea how to fix this.
There was a problem hiding this comment.
@Krishna2323 Let's call canUserPerformWriteAction function in components instead of in the shouldReportActionBeVisible utility function and then pass a boolean to shouldReportActionBeVisible
There was a problem hiding this comment.
@rayane-djouah, I found using reportID cleaner instead of using canUserPerformWriteAction in components and then passing it to shouldReportActionBeVisible but let me know if you still think the other way. I will make those changes.
There was a problem hiding this comment.
We can't use functions from ReportUtils in ReportActionsUtils because ReportUtils depends on functions from ReportActionsUtils, which would create a circular dependency.
There was a problem hiding this comment.
@rayane-djouah, should we create the same functions in ReportActionsUtils?
There was a problem hiding this comment.
This would result in code duplication. According to the author and reviewer checklist:
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
Therefore, we should avoid duplicating functions.
There was a problem hiding this comment.
Let's call
canUserPerformWriteActionfunction in components instead of in the shouldReportActionBeVisible utility function and then pass a boolean to shouldReportActionBeVisible
@rayane-djouah, then we can do this but then we have to get the report using getReport function in every file which only have the reportID and then we can use canUserPerformWriteAction and pass the result to shouldReportActionBeVisible. WDYT?
There was a problem hiding this comment.
Using getReport function (or useOnyx hook) sounds good to me 👍
There was a problem hiding this comment.
sorry for delay, will provide updates within 24hours.
|
@Krishna2323 Could you please share an update? Thanks! |
src/libs/ReportActionsUtils.ts
Outdated
| ) { | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
@rayane-djouah, if we pass canUserPerformWriteAction boolean to shouldReportActionBeVisible then how we will get the reportID for isActionableJoinRequestPending, should we pass both?
There was a problem hiding this comment.
@Krishna2323 Can we use reportAction?.reportID instead?
There was a problem hiding this comment.
@rayane-djouah, reportAction?.reportID is undefined.
|
@Krishna2323 Let's pass both parameters. We also have conflicts. |
|
@Krishna2323 Friendly bump |
|
Sorry for delay, I will provide updates today. |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@Krishna2323 Performance Tests are failing |
src/libs/ReportActionsUtils.ts
Outdated
| */ | ||
| function shouldReportActionBeVisible(reportAction: OnyxEntry<ReportAction>, key: string | number): boolean { | ||
| function shouldReportActionBeVisible(reportAction: OnyxEntry<ReportAction>, key: string | number, reportID: string, canUserPerformWriteAction?: boolean): boolean { | ||
| if ((isActionableReportMentionWhisper(reportAction) || isActionableJoinRequestPending(reportID ?? '-1') || isActionableMentionWhisper(reportAction)) && !canUserPerformWriteAction) { |
There was a problem hiding this comment.
Reviewing the code here:
App/src/libs/ReportActionsUtils.ts
Lines 1480 to 1502 in bef062b
I suggest we create a function named isActionableJoinRequestPendingReportAction and refactor the code accordingly:
function isActionableJoinRequestPendingReportAction(reportAction: OnyxEntry<ReportAction>): boolean {
return isActionableJoinRequest(reportAction) && getOriginalMessage(reportAction)?.choice === ('' as JoinWorkspaceResolution);
}
function getActionableJoinRequestPendingReportAction(reportID: string): OnyxEntry<ReportAction> {
const findPendingRequest = Object.values(getAllReportActions(reportID)).find((reportActionItem) => isActionableJoinRequestPendingReportAction(reportActionItem));
return findPendingRequest;
}Then use isActionableJoinRequestPendingReportAction in the following manner:
| if ((isActionableReportMentionWhisper(reportAction) || isActionableJoinRequestPending(reportID ?? '-1') || isActionableMentionWhisper(reportAction)) && !canUserPerformWriteAction) { | |
| if ((isActionableReportMentionWhisper(reportAction) || isActionableJoinRequestPendingReportAction(reportAction) || isActionableMentionWhisper(reportAction)) && !canUserPerformWriteAction) { |
This refactor eliminates the need for the reportID parameter.
| if (!reportAction) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Let's keep this if block first
| ) { | ||
| const targetNote = privateNotes?.[Number(accountID)]?.note ?? ''; | ||
| const attachments: Attachment[] = []; | ||
| const report = ReportUtils.getReport(reportID); |
There was a problem hiding this comment.
| const report = ReportUtils.getReport(reportID); | |
| const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`); |
There was a problem hiding this comment.
extractAttachments is a util function so we can't use useOnyx here.
|
|
||
| const sortedReportActions = ReportActionsUtils.getSortedReportActionsForDisplay(itemReportActions, reportID, ReportUtils.canUserPerformWriteAction(itemFullReport)); |
There was a problem hiding this comment.
| const sortedReportActions = ReportActionsUtils.getSortedReportActionsForDisplay(itemReportActions, reportID, ReportUtils.canUserPerformWriteAction(itemFullReport)); | |
| const canUserPerformWriteAction = ReportUtils.canUserPerformWriteAction(itemFullReport); | |
| const sortedReportActions = ReportActionsUtils.getSortedReportActionsForDisplay(itemReportActions, reportID, canUserPerformWriteAction); |
| const {workspaceName, reportName} = parentNavigationSubtitleData; | ||
| const {isOffline} = useNetwork(); | ||
| const {translate} = useLocalize(); | ||
| const report = ReportUtils.getReport(parentReportID); |
There was a problem hiding this comment.
| const report = ReportUtils.getReport(parentReportID); | |
| const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`); |
| // Use `||` instead of `??` to handle empty string. | ||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
| const reportIDWithDefault = reportID || '-1'; | ||
| const report = ReportUtils.getReport(reportIDWithDefault); |
There was a problem hiding this comment.
| const report = ReportUtils.getReport(reportIDWithDefault); | |
| const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`); |
There was a problem hiding this comment.
usePaginatedReportActions is a util function so we can't use useOnyx here.
There was a problem hiding this comment.
useOnyx is already a part of this file, so I assume it works just fine? Generally, ReportUtils.getReport() should be avoided because anything that it returns won't get updated if the onyx data updates.
There was a problem hiding this comment.
Sorry my mistake, now updated.
| function DebugReportActions({reportID}: DebugReportActionsProps) { | ||
| const {translate, datetimeToCalendarTime} = useLocalize(); | ||
| const styles = useThemeStyles(); | ||
| const report = ReportUtils.getReport(reportID); |
There was a problem hiding this comment.
| const report = ReportUtils.getReport(reportID); | |
| const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`); |
|
I will work on the suggested changes today. |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-11-13.at.5.21.14.PM.movAndroid: mWeb ChromeScreen.Recording.2024-11-13.at.5.23.59.PM.moviOS: NativeSimulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-11-13.at.17.16.28.mp4iOS: mWeb SafariSimulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-11-13.at.17.19.37.mp4MacOS: Chrome / SafariScreen.Recording.2024-11-13.at.5.04.57.PM.movMacOS: DesktopScreen.Recording.2024-11-13.at.5.08.43.PM.mov |
tgolen
left a comment
tgolen
left a comment
There was a problem hiding this comment.
Is there already a unit test explicitly for what is being fixed here? If not, please add one.
| // Use `||` instead of `??` to handle empty string. | ||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
| const reportIDWithDefault = reportID || '-1'; | ||
| const report = ReportUtils.getReport(reportIDWithDefault); |
There was a problem hiding this comment.
useOnyx is already a part of this file, so I assume it works just fine? Generally, ReportUtils.getReport() should be avoided because anything that it returns won't get updated if the onyx data updates.
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
@Krishna2323 Let's add a test case (for actionable whispers in archived reports) in tests/unit/ReportActionsUtilsTest.ts for App/tests/unit/ReportActionsUtilsTest.ts Line 313 in 9228354 App/tests/unit/ReportActionsUtilsTest.ts Line 208 in 9228354 App/tests/unit/ReportActionsUtilsTest.ts Line 408 in 9228354 |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@rayane-djouah test case added for for actionable whispers in archived reports. |
tests/unit/ReportActionsUtilsTest.ts
Outdated
| expect(result).toStrictEqual(input); | ||
| }); | ||
|
|
||
| it('should filter whisper action in a archived report', () => { |
There was a problem hiding this comment.
Can you please follow the directions here for adding good comments to the tests?
| }); | ||
|
|
||
| it('should filter whisper action in a archived report', () => { | ||
| const input: ReportAction[] = [ |
There was a problem hiding this comment.
Is all this data necessary for the test?
There was a problem hiding this comment.
@tgolen, yes, it's necessary for checking that other types of actions aren't being filtered.
edit: I have updated the input data according to the test.
There was a problem hiding this comment.
OK, I can see how it would be helpful to have some non-actionable whisper action types, but I don't think it's necessary to have all of CREATED, ADD_COMMENT, and CLOSED. I would suggest only having one of them (maybe ADD_COMMENT since that is pretty typical).
If you feel that CREATED and CLOSED is necessary to include, please explain it better why they are necessary as maybe I don't fully understand it.
There was a problem hiding this comment.
CREATED and CLOSED type actions aren't necessary, removed them from the input and only kept ADD_COMMENT.
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
tests/unit/ReportActionsUtilsTest.ts
Outdated
|
|
||
| const result = ReportActionsUtils.getSortedReportActionsForDisplay(input, false); | ||
| // Expected output should filter out actionable whisper actions type e.g. "join", "create room" | ||
| // because "canUserPerformWriteAction" is false (report is archived) |
There was a problem hiding this comment.
@rayane-djouah @tgolen, are these comments enough for the explanation? Sorry for the trouble, I'm writing unit tests for the first time :(
There was a problem hiding this comment.
No worries! You'll get there. A few things:
These comments don't match the instructions I had you look at (it's OK, these are new instructions that we're trying for the first time, so you're also helping me improve the instructions!)
Can you please:
- Add three separate comments, each for the "Given", "When", and "Then" sections
- Have each comment explain WHY the code is there like it is
- Look at other tests in the code using this format to get some ideas (search for
// Given). They won't always be the best examples, but you'll get a better idea of what I'm asking for
I'll add some suggestions to your tests here to explain what I mean
There was a problem hiding this comment.
Thanks for the help 🙏🏻. I now have a better understanding of how the "Given," "When," and "Then" sections should be written. I’ve updated the comments (mostly copied from your suggestions 🫠).
tests/unit/ReportActionsUtilsTest.ts
Outdated
|
|
||
| const result = ReportActionsUtils.getSortedReportActionsForDisplay(input, false); | ||
| // Expected output should filter out actionable whisper actions type e.g. "join", "create room" | ||
| // because "canUserPerformWriteAction" is false (report is archived) |
There was a problem hiding this comment.
No worries! You'll get there. A few things:
These comments don't match the instructions I had you look at (it's OK, these are new instructions that we're trying for the first time, so you're also helping me improve the instructions!)
Can you please:
- Add three separate comments, each for the "Given", "When", and "Then" sections
- Have each comment explain WHY the code is there like it is
- Look at other tests in the code using this format to get some ideas (search for
// Given). They won't always be the best examples, but you'll get a better idea of what I'm asking for
I'll add some suggestions to your tests here to explain what I mean
| }); | ||
|
|
||
| it('should filter actionable whisper actions e.g. "join", "create room" when room is archived', () => { | ||
| const input: ReportAction[] = [ |
There was a problem hiding this comment.
| const input: ReportAction[] = [ | |
| // Given several different action types, including actionable whispers for creating and joining rooms, as well as non-actionable whispers | |
| // - CREATED | |
| // - ADD_COMMENT | |
| // - ACTIONABLE_REPORT_MENTION_WHISPER | |
| // - ACTIONABLE_MENTION_WHISPER | |
| // - CLOSED | |
| const input: ReportAction[] = [ |
tests/unit/ReportActionsUtilsTest.ts
Outdated
| // Expected output should filter out actionable whisper actions type e.g. "join", "create room" | ||
| // because "canUserPerformWriteAction" is false (report is archived) |
There was a problem hiding this comment.
| // Expected output should filter out actionable whisper actions type e.g. "join", "create room" | |
| // because "canUserPerformWriteAction" is false (report is archived) | |
| // Then the output should correctly filter out the actionable whisper types for "join" and "create room" | |
| // because the report is archived and making those actions not only doesn't make sense from a UX standpoint, but leads to server errors since those actions aren't possible. |
tests/unit/ReportActionsUtilsTest.ts
Outdated
| // Expected output should filter out actionable whisper actions type e.g. "join", "create room" | ||
| // because "canUserPerformWriteAction" is false (report is archived) | ||
| // eslint-disable-next-line rulesdir/prefer-at | ||
| const expectedOutput: ReportAction[] = [...input.slice(1, 3), input[0]]; |
There was a problem hiding this comment.
[...input.slice(1, 3), input[0]]; is not very clear that it's specifically filtering out the actionable whispers. I suggest modifying this to use .filter() and filter based on the actionName.
There was a problem hiding this comment.
updated to use filter() method.
| }); | ||
|
|
||
| it('should filter whisper action in a archived report', () => { | ||
| const input: ReportAction[] = [ |
There was a problem hiding this comment.
OK, I can see how it would be helpful to have some non-actionable whisper action types, but I don't think it's necessary to have all of CREATED, ADD_COMMENT, and CLOSED. I would suggest only having one of them (maybe ADD_COMMENT since that is pretty typical).
If you feel that CREATED and CLOSED is necessary to include, please explain it better why they are necessary as maybe I don't fully understand it.
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
tgolen
left a comment
tgolen
left a comment
There was a problem hiding this comment.
That looks really good now! Nice work. I just noticed this one last thing.
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
tgolen
left a comment
tgolen
left a comment
There was a problem hiding this comment.
Looks very good! Thank you!
|
✋ 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: 9.0.65-0 🚀
|
|
🚀 Deployed to production by https://github.com/chiragsalian in version: 9.0.65-5 🚀
|
| ReportActionsUtils.shouldReportActionBeVisible(reportAction, reportAction.reportActionID, ReportUtils.canUserPerformWriteAction(report)), | ||
| ), | ||
| [sortedReportActions, isOffline], | ||
| [sortedReportActions, isOffline, report], |
There was a problem hiding this comment.
Having the report here was retriggering the memo block multiple times, which caused this blocker #52891
| // Use `||` instead of `??` to handle empty string. | ||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
| const reportIDWithDefault = reportID || '-1'; | ||
| const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`); |
There was a problem hiding this comment.
FYI, we need to use reportIDWithDefault here. This caused this issue: #52864
5 participants
Explanation of Change
shouldReportActionBeVisibleto also includecanUserPerformWriteActioncheck for the whisper actions.Fixed Issues
$ #49940
PROPOSAL: #49940 (comment)
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.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_native.mp4
Android: mWeb Chrome
android_chrome.mp4
iOS: Native
ios_native.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web_chrome.mp4
MacOS: Desktop
desktop_app.mp4