[No QA] Remove a call to getReportNameValuePairs from shouldShowFlagComment()#65204
Conversation
|
@cristipaval 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] |
|
@DylanDylann Can you please give a C+ review on this? |
| tag: randWord(), | ||
| parentTransactionID: index.toString(), | ||
| status: rand(Object.values(CONST.TRANSACTION.STATUS)), | ||
| status: rand(Object.values(CONST.TRANSACTION.STATUS)) as Transaction['status'], |
There was a problem hiding this comment.
I was getting some type errors, and this seemed to fix it. If you'd like me to undo it and come up with a better plan, please let me know.
There was a problem hiding this comment.
I think it happens to you because some your local setup. I don't see type errors in these places and our GitHub action also doesn't alert to any error. So I think we should revert these changes
There was a problem hiding this comment.
OK, thanks for checking. I'll revert them.
| await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${chatReport.reportID}`, null); | ||
| }); | ||
|
|
||
| it('should return false for an archived chatreport', () => { |
There was a problem hiding this comment.
| it('should return false for an archived chatreport', () => { | |
| it('should return false for an archived chat report', () => { |
| actorAccountID: 123456, | ||
| }; | ||
|
|
||
| describe('can flag report action', () => { |
There was a problem hiding this comment.
I think we should add unit tests for canFlagReportAction function and hard-code value of this function here
There was a problem hiding this comment.
That's a good idea. I'll work on that today.
There was a problem hiding this comment.
and hard-code value of this function here
Actually, I'm not 100% sure what you mean by this. What would get hard-coded? I think shouldShowFlagComment() would still need two cases:
- A report action that can be flagged
- A report action that cannot be flagged
Since canFlagReportAction() is only called inside of shouldShowFlagComment(), there wouldn't be anything to hard code in the tests.
There was a problem hiding this comment.
@tgolen canFlagReportAction() is called in three places so If possible, I think we should test this function independently
| // It will be true here because the report action can be flagged since it was a whisper from someone that is not Concierge | ||
| // It doesn't matter if the expense report is archived or not |
There was a problem hiding this comment.
These comments are unnecessary
There was a problem hiding this comment.
Heh, I was having a hard time wrapping my head around all the negated logic, so I found them helpful, but I'll remove them if they aren't helpful.
| it('should return false for an archived chat report', () => { | ||
| // It will be false here because it's a chat report with Chronos and therefore | ||
| // it doesn't matter if the report is archived or not, it will always be false. | ||
| expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(false); | ||
| }); | ||
|
|
||
| it('should return false for a non-archived chat report', () => { | ||
| // It will be false here because it's a chat report with Chronos and therefore | ||
| // it doesn't matter if the report is archived or not, it will always be false. | ||
| expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(false); | ||
| }); |
There was a problem hiding this comment.
Ah, that first one should pass true for the third parameter.
| it('should return false for an archived chat report', () => { | ||
| // It will be false here because it's a chat report with Concierge and therefore | ||
| // it doesn't matter if the report is archived or not, it will always be false. | ||
| expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(false); | ||
| }); | ||
|
|
||
| it('should return false for a non-archived chat report', () => { | ||
| // It will be false here because it's a chat report with Concierge and therefore | ||
| // it doesn't matter if the report is archived or not, it will always be false. | ||
| expect(shouldShowFlagComment(validReportAction, chatReport, false)).toBe(false); | ||
| }); |
There was a problem hiding this comment.
Same here, the first one should pass true for the third parameter.
| }); | ||
| }); | ||
|
|
||
| describe('chat reports', () => { |
There was a problem hiding this comment.
We already had UTs for isArchivedNonExpenseReport function so I don't think we need to have this case here
There was a problem hiding this comment.
True, that's a good point.
| }); | ||
| }); | ||
|
|
||
| describe('expense reports', () => { |
There was a problem hiding this comment.
We already had UTs for isArchivedNonExpenseReport function so I don't think we need to have this case here
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
|
I haven't had time to work on this today, but I am hoping to get to it tomorrow to add full coverage unit tests to |
|
I was able to update this PR with complete unit test coverage for |
DylanDylann
left a comment
DylanDylann
left a comment
There was a problem hiding this comment.
LGTM. I don't understand why the check spell action failed. @tgolen Could you merge main again or trigger this action again?
|
🎯 @DylanDylann, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #65399. |
|
OK, I've updated from main again. Let's see what happens
…On Thu, Jul 3, 2025 at 2:53 AM melvin-bot[bot] ***@***.***> wrote:
*melvin-bot[bot]* left a comment (Expensify/App#65204)
<#65204 (comment)>
🎯 @DylanDylann <https://github.com/DylanDylann>, thanks for reviewing
and testing this PR! 🎉
An E/App issue has been created to issue payment here: #65399
<#65399>.
—
Reply to this email directly, view it on GitHub
<#65204 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJMAB6LLXQPAAZ3VA6KNLL3GTOODAVCNFSM6AAAAACAPDK2U2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTAMZRGI2TMNBTHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Looks good but lots of failing jest / lint checks. Are those unrelated? |
|
Hm, not sure. I'll dig into those today. |
| }); | ||
|
|
||
| it('cannot be flagged if the report does not exist', () => { | ||
| expect(canFlagReportAction(nonWhisperReportAction, 'starwarsisthebest')).toBe(false); |
There was a problem hiding this comment.
| expect(canFlagReportAction(nonWhisperReportAction, 'starwarsisthebest')).toBe(false); | |
| expect(canFlagReportAction(nonWhisperReportAction, '1234')).toBe(false); |
|
OK, this should be all good now. Tests are all passing. |
|
✋ 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/blimpich in version: 9.1.77-1 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.1.77-2 🚀
|
4 participants
Explanation of Change
This PR makes
shouldShowFlagComment()a pure function.It also adds unit tests for all the different logic cases.
Fixed Issues
Part of #59961
Tests
None. It's all covered by unit tests on existing functionality so functional testing isn't necessary.
Offline tests
None
QA Steps
None
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop