Always show green Submit badge/button when report is ready to submit#89889
Conversation
…less of violations LHN: When a report requires attention and the green badge is Submit, prioritize the green Submit GBR over the red Fix RBR — even when violations exist. Report preview: Always render the Submit button as green (success) since it's only shown when the report is submittable. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
Non-submitters (approvers, admins) now see the gray View button instead of the green Submit button on the report preview. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
@MelvinBot pull main. Add Tests steps |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
Added Tests, Offline tests, and QA steps to the PR description. Re: pull main — The merge failed due to unrelated git histories (the branch doesn't share a common ancestor with |
Restructure the Submit-badge-priority logic to use the original sequential if-block pattern instead of an if/else wrapper. This maintains the same execution flow that the perf test baseline expects while still achieving the desired behavior: when a report is ready to submit, always show the green Submit badge regardless of violations. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
⏱️ Ran out of time before I could finish. This usually means the task was too complex or I got stuck exploring. Please try again with a more specific request or post in #melvinbot-ai. |
|
⏱️ Ran out of time before I could finish. This usually means the task was too complex or I got stuck exploring. Please try again with a more specific request or post in #melvinbot-ai. |
|
The only real failing check is perf-tests (Reassure) — the Perf-tests failure: Analysis: After investigation, this appears to be a flaky CI issue, not caused by the PR changes:
Recommendation: Re-run the perf-tests check. If it fails again consistently, it may need investigation by the performance team as a broader main-branch issue. |
|
@MelvinBot pull main |
…mitBadge Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
Merged |
aimane-chnaif
left a comment
aimane-chnaif
left a comment
There was a problem hiding this comment.
@MelvinBot add unit tests
| // When the report is ready to submit, always show the green Submit badge | ||
| // regardless of violations — the user should submit first, then fix. |
There was a problem hiding this comment.
@MelvinBot update the user should submit first, then fix. to the user can submit without fix.
There was a problem hiding this comment.
Done — updated to "the user can submit without fix."
| // see it on the parent workspace chat. The parent still gets ERROR/FIX through the | ||
| // propagation loop below, so the actionable indicator surfaces on the workspace chat row | ||
| // (which is where C+ wants it). Skips when the chat parent isn't accessible to the user. | ||
| // Also skip setting ERROR when we'll show the green Submit badge — let the user submit first. |
There was a problem hiding this comment.
@MelvinBot update let the user submit first. to let the user submit without fix.
There was a problem hiding this comment.
Done — updated to "let the user submit without fix."
| const chatPolicy = chatReport?.policyID ? policies?.[`${ONYXKEYS.COLLECTION.POLICY}${chatReport.policyID}`] : undefined; | ||
| const isChildOfAccessiblePolicyExpenseChat = !!chatReport && isPolicyExpenseChat(chatReport) && (!!chatReport.isOwnPolicyExpenseChat || isPolicyAdmin(chatPolicy)); | ||
| if (!isChildOfAccessiblePolicyExpenseChat) { | ||
| if (!isChildOfAccessiblePolicyExpenseChat && !willShowGreenSubmit) { |
There was a problem hiding this comment.
@MelvinBot for optimization, move && !willShowGreenSubmit condition to if (reasonAndReportAction) {, along with comment
There was a problem hiding this comment.
Done — moved && !willShowGreenSubmit to the outer if (reasonAndReportAction && !willShowGreenSubmit) check, which skips the entire block (including needsParentChatErrorPropagation) when the green Submit badge will be shown.
Add two tests verifying that managers and admins no longer see the
Submit action on open expense reports — only the report submitter does.
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
|
Added two unit tests to |
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
| // see it on the parent workspace chat. The parent still gets ERROR/FIX through the | ||
| // propagation loop below, so the actionable indicator surfaces on the workspace chat row | ||
| // (which is where C+ wants it). Skips when the chat parent isn't accessible to the user. | ||
| // Also skip setting ERROR when we'll show the green Submit badge — let the user submit without fix. |
There was a problem hiding this comment.
Done — moved the comment above the if block.
Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
@MelvinBot complete author checklist |
|
⏱️ Ran out of time before I could finish. This usually means the task was too complex or I got stuck exploring. Please try again with a more specific request or post in #melvinbot-ai. |
|
@MelvinBot mark as ready for review |
|
Marked this PR as ready for review. ✅ |
melvin-bot
Bot
requested review from
flaviadefaria
and removed request for
a team
|
@aimane-chnaif 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: HybridAppandroid-admin.movAndroid: mWeb ChromeiOS: HybridAppios-submit.blocked.moviOS: mWeb Safarimsafari-approver.movMacOS: Chrome / Safarisubmitter.movadmin.approver.mov |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #89444 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
Code ReviewOverall the changes are clean and well-scoped. The Potential issue:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b98fa401be
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // if report has errors or violations, show red dot | ||
| if (reasonAndReportAction) { | ||
| // Also skip setting ERROR when we'll show the green Submit badge — let the user submit without fix. | ||
| if (reasonAndReportAction && !willShowGreenSubmit) { |
There was a problem hiding this comment.
Narrow the willShowGreenSubmit bypass so it only suppresses violation-based RBR/Fix states, not every reasonAndReportAction. As written, any red-brick reason (including HAS_ERRORS from failed actions/report errors) is skipped whenever the report is submit-ready, which hides real failure indicators and disables parent error propagation for those cases. This can leave users seeing a green Submit badge even when the report has unresolved errors that previously surfaced as red.
Useful? React with 👍 / 👎.
|
AI reviews above are not valid.
|
|
Confirmed — the Codex review concern is invalid. The logic chain guarantees there are no blocking violations when
So the |
|
@luacmartins looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
🚧 @luacmartins has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
4 participants
Explanation of Change
Report preview button behavior:
Submitbutton (regardless of violations)ViewbuttonLHN (reportAttributes.ts): When
requiresAttentionis true and the green badge isSUBMIT, the green Submit GBR now takes priority over the red Fix RBR. Previously, violations would override the Submit badge with a red Fix badge, which was confusing because the user should submit first and fix violations later.Report preview (SubmitActionButton.tsx): The Submit button is now always rendered with the green
successstyle. Since only the submitter sees this component, it is always green.Report preview (ReportPreviewActionUtils.ts): The
canSubmitcheck now only returns true for the report submitter (owner). Managers and admins no longer see the Submit button — they see the default gray View button instead.Fixed Issues
$ #89444
Tests
Prerequisites: You need two accounts — one as the submitter (report owner) and one as the approver/admin in a workspace with an approval workflow enabled.
Submitbadge in the LHN — even if the report has violationsFixbadgeSubmitbuttonViewbutton on the report preview (not a Submit button)Offline tests
Submitfor submitter)QA Steps
Submitbadge (not redFix)SubmitbuttonViewbuttonPR 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))npm run compress-svg)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