Skip to content
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

[$500] Delete request confirmation modal has Delete comment as the title #28861

Closed
6 tasks done
trjExpensify opened this issue Oct 4, 2023 · 25 comments
Closed
6 tasks done
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review

Comments

@trjExpensify
Copy link
Contributor

trjExpensify commented Oct 4, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Action Performed:

  1. Create a request
  2. Navigate to the expense/iouReport
  3. Hover over the request > click the trash icon on the comment action menu
  4. Observe the title of the modal reads Delete comment

Expected Result:

The delete confirmation modal should read Delete request

Actual Result:

The delete confirmation modal reads Delete comment

Workaround:

N/A

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: v1.3.77-5
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

3o5sNkB2p9.mp4

Expensify/Expensify Issue URL:
Issue reported by: @trjExpensify
Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01677c1677ad8e585b
  • Upwork Job ID: 1709667849059155968
  • Last Price Increase: 2023-10-04
  • Automatic offers:
    • aimane-chnaif | Reviewer | 27107636
@trjExpensify trjExpensify added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Oct 4, 2023
@trjExpensify trjExpensify self-assigned this Oct 4, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 4, 2023

Current assignee @trjExpensify is eligible for the Bug assigner, not assigning anyone new.

@melvin-bot melvin-bot bot changed the title Delete request confirmation modal has Delete comment as the title [$500] Delete request confirmation modal has Delete comment as the title Oct 4, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 4, 2023

Job added to Upwork: https://www.upwork.com/jobs/~01677c1677ad8e585b

@melvin-bot
Copy link

melvin-bot bot commented Oct 4, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 4, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 4, 2023

Triggered auto assignment to Contributor-plus team member for initial proposal review - @aimane-chnaif (External)

@neonbhai
Copy link
Contributor

neonbhai commented Oct 4, 2023

The title in video says Delete request (as expected?) 🤔

@ikevin127
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue

The delete confirmation modal reads Delete comment.

What is the root cause of that problem?

The problem is that within the PopoverReportActionContextMenu.js component:

https://github.com/Expensify/App/blob/ef3073066be6855bed47088b29e99a405e1d0feb/src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.js#L318C1-L333C7

The ConfirmModal title passes empty object reportAction to the reportActionContextMenu.deleteAction translate function which causes the function to render 'comment' because action value is false:

https://github.com/Expensify/App/blob/ef3073066be6855bed47088b29e99a405e1d0feb/src/languages/en.ts#L403C1-L403C141

What changes do you think we should make in order to solve the problem?

Within the PopoverReportActionContextMenu.js component pass the correct title by passing {action: reportAction} like it is passed below for the prompt:

            <ConfirmModal
-               title={translate('reportActionContextMenu.deleteAction', {reportAction})}
+               title={translate('reportActionContextMenu.deleteAction', {action: reportAction})}
                isVisible={isDeleteCommentConfirmModalVisible}
                shouldSetModalVisibility={shouldSetModalVisibilityForDeleteConfirmation}
                onConfirm={confirmDeleteAndHideModal}
                onCancel={hideDeleteModal}
                onModalHide={() => {
                    callbackWhenDeleteModalHide.current();
                }}
                prompt={translate('reportActionContextMenu.deleteConfirmation', {action: reportAction})}
                confirmText={translate('common.delete')}
                cancelText={translate('common.cancel')}
                danger
            />

What alternative solutions did you explore? (Optional)

N/A

Videos

web.mp4

@trjExpensify
Copy link
Contributor Author

trjExpensify commented Oct 4, 2023

The title in video says Delete request (as expected?) 🤔

Ah, you're looking at the second place in the video you can see this same modal where it is indeed correct. Look at the first one shown at 0.10s where it's incorrect.

@PiyushChandra17
Copy link

Proposal

Please re-state the problem that we are trying to solve in this issue.

Delete request confirmation modal has Delete comment as the title

What is the root cause of that problem?

"action" key is missing in title prop of ConfirmModal, here

title={translate('reportActionContextMenu.deleteAction', {reportAction})}

What changes do you think we should make in order to solve the problem?

We need to add action key here,

title={translate('reportActionContextMenu.deleteAction', {action: reportAction})} as we have specified in prompt prop

<ConfirmModal
    title={translate('reportActionContextMenu.deleteAction', {action: reportAction})}
    isVisible={isDeleteCommentConfirmModalVisible}
    shouldSetModalVisibility={shouldSetModalVisibilityForDeleteConfirmation}
    onConfirm={confirmDeleteAndHideModal}
    onCancel={hideDeleteModal}
    onModalHide={() => {
        callbackWhenDeleteModalHide.current();
    }}
    prompt={translate('reportActionContextMenu.deleteConfirmation', {action: reportAction})}
    confirmText={translate('common.delete')}
    cancelText={translate('common.cancel')}
    danger
/>

What alternative solutions did you explore? (Optional)

NA

Result:

Screen.Recording.2023-10-05.at.11.37.39.AM.mov

@DylanDylann
Copy link
Contributor

This is regression from #27221

@melvin-bot melvin-bot bot added the Overdue label Oct 9, 2023
@trjExpensify
Copy link
Contributor Author

@DylanDylann is that to say it's being fixed elsewhere as well? CC: @OlimpiaZurek

@melvin-bot melvin-bot bot removed the Overdue label Oct 9, 2023
@ikevin127
Copy link
Contributor

This issue is still reproductible on current staging / local (latest main branch).

Hopefully this will be treated soon as a regression from this PR.
Otherwise my proposal is ready for review and the PR can't wait to come into existence!

@OlimpiaZurek
Copy link
Contributor

I just opened the PR with the fix.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 9, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 9, 2023

📣 @aimane-chnaif 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

@melvin-bot melvin-bot bot added the Overdue label Oct 12, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 13, 2023

@trjExpensify, @OlimpiaZurek, @aimane-chnaif Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@aimane-chnaif
Copy link
Contributor

Not overdue

@melvin-bot melvin-bot bot removed the Overdue label Oct 13, 2023
@melvin-bot melvin-bot bot added the Overdue label Oct 16, 2023
@trjExpensify trjExpensify added the Reviewing Has a PR in review label Oct 16, 2023
@melvin-bot melvin-bot bot removed the Overdue label Oct 16, 2023
@trjExpensify
Copy link
Contributor Author

Let's make sure we add the reviewing label if it's missing.

@melvin-bot
Copy link

melvin-bot bot commented Oct 23, 2023

@trjExpensify, @OlimpiaZurek, @aimane-chnaif Whoops! This issue is 2 days overdue. Let's get this updated quick!

@trjExpensify
Copy link
Contributor Author

Is something wrong with the PR here. It went to staging two weeks ago apparently, but no melvin notice for prod? 🤔

@melvin-bot
Copy link

melvin-bot bot commented Oct 31, 2023

@trjExpensify, @OlimpiaZurek, @aimane-chnaif Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@trjExpensify
Copy link
Contributor Author

Bump on the Q above. :)

Copy link

melvin-bot bot commented Nov 7, 2023

@trjExpensify, @OlimpiaZurek, @aimane-chnaif Whoops! This issue is 2 days overdue. Let's get this updated quick!

@trjExpensify
Copy link
Contributor Author

@OlimpiaZurek @aimane-chnaif this is on prod, so I'm really not sure what happened there. @aimane-chnaif confirming you require payment of $500 for the PR review correct?

@aimane-chnaif
Copy link
Contributor

Correct. Not sure what's happening too. There was similar case at that moment. I believe this is on prod as well.

@trjExpensify
Copy link
Contributor Author

Okay offer has been sent. 👍

@trjExpensify
Copy link
Contributor Author

Settled up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

7 participants