-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Hide approve/pay buttons if the report contains violations #51133
Hide approve/pay buttons if the report contains violations #51133
Conversation
@hoangzinh 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] |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-11-20.at.17.51.14.android.movAndroid: mWeb ChromeScreen.Recording.2024-11-20.at.17.48.45.android.chrome.moviOS: NativeScreen.Recording.2024-11-20.at.17.52.55.ios.moviOS: mWeb SafariScreen.Recording.2024-11-20.at.17.54.22.ios.safari.movMacOS: Chrome / SafariScreen.Recording.2024-11-20.at.17.38.06.web.movMacOS: DesktopScreen.Recording.2024-11-20.at.17.45.10.desktop.mov |
@abzokhattab can you recheck your recordings? |
oh sorry i needed to insert a new line for the images to render
Should we also hide the pay button if there is a violation ? |
Sorry for late response, I thought we won't' show pay button until an expense is approved 👀 |
Hi @abzokhattab I finally found it. If you're policy owner/admin and also an approver, then instead of hide both Screen.Recording.2024-10-31.at.05.41.56.movI think it's related to this logic Line 7102 in 30b0b8d
|
Good catch! Thank you, @hoangzinh, for identifying this issue. I’ve disabled the pay button whenever the report contains violations. |
Bug 2: App still shows the "Approve" button in the report screen, it will hide when you go to IOU details then go back to the report screen Screen.Recording.2024-11-04.at.06.31.21.mov |
cc @abzokhattab on the above bug ^ |
…n-if-expense-has-violatiosn
i tried couple of times to reproduce the previous bug but i cannot ... i always get the submit button after submitting the report then after submission no other buttons are shown... is there a step am i missing? |
It's not always reproducible @abzokhattab. I'm guessing because the component doesn't subscribe to Onyx |
i see .. but the thing is i am not able to reproduce it so i cant tell how it could be solved, i always get the submit button after submitting the report unlike your case (the approve btn). is there a specific reproduction step i am missing? |
It can be reproducible in native apps (specific Android). Step to reproduce:
Example: #51133 (comment) |
following those steps i am still not able to reproduce your case.. can you please recheck that its still reproducible? ios Screen.Recording.2024-11-12.at.01.02.07.movandroid Untitled.mov |
It's weird that I can still reproduce it in both Android & IOS. Can you confirm if you're logged in as approver in Android/IOS device? Also can you take a step further by go to IOU, then fix error and verify if the Approve button is displayed? Thank you. Screen.Recording.2024-11-12.at.17.25.22.mov |
Sorry for the late response ... i had an emergency the last few days ... Yes i am logged in as an approver ... here is the full rerproduction steps starting from creating a workspace and inviting the other user ... please let me know which step is wrong in this case: Screen.Recording.2024-11-18.at.01.29.41.mov |
Hi @abzokhattab can you change to "auto-submit" then in the approver device (mobile device)? And can you go to IOU page and then fix error to see if the approve button appears after resolving all errors? |
I am now able to reproduce it ... i agree we should pass the violations object as a param to the can approve and pay functions.. the bug is fixed on my side after this change ... can you please double check |
Looks good. Thanks @abzokhattab I will review today |
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.
LGTM
There are some conflicts @abzokhattab |
…n-if-expense-has-violatiosn
thanks @Gonals, i have just resolved the conflicts |
We have conflicts again 😅 |
…n-if-expense-has-violatiosn
ops .. just merged again :D |
@@ -289,7 +289,7 @@ function getAction(data: OnyxTypes.SearchResults['data'], key: string): SearchTr | |||
const chatReportRNVP = data[`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${report?.chatReportID}`] ?? undefined; | |||
|
|||
if ( | |||
IOU.canIOUBePaid(report, chatReport, policy, allReportTransactions, false, chatReportRNVP, invoiceReceiverPolicy) && | |||
IOU.canIOUBePaid(report, chatReport, policy, allReportTransactions, undefined, false, chatReportRNVP, invoiceReceiverPolicy) && |
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.
Is there any util that returns transaction violations? So we can get and pass here.
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.
We can use oynx.connect
to link to the transaction violations at the top of the file. However, from a performance perspective, I thought it would be better to make it optional and use the existing one in the IOU file as a fallback.
let me know if you think we should use the oynx.connect
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.
No worry. I think it's fine for now. Can you do a quick test to ensure everything still good?
✋ 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/Gonals in version: 9.0.67-0 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.67-9 🚀
|
🚀 Deployed to staging by https://github.com/Gonals in version: 9.0.68-0 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.68-7 🚀
|
Details
Fixed Issues
$ #50479
PROPOSAL: #50479 (comment)
Tests
Offline tests
Same as tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop