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

[HOLD for payment 2023-11-30][$500][ECARD] CRITICAL: Settings - Add “Get physical card” button and necessary routes #22877

Closed
grgia opened this issue Jul 14, 2023 · 32 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 ECard Wave5-free-submitters Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review

Comments

@grgia
Copy link
Contributor

grgia commented Jul 14, 2023

HOLD ON #22873

Design Doc Section

https://docs.google.com/document/d/1rFxJ78vz5On6zSWzYa51B9v-tyLdC5pUsBeLOLig0t4/edit#bookmark=id.rl1asurfa7yq

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~013b675735e78d2be9
  • Upwork Job ID: 1679791341527105536
  • Last Price Increase: 2023-12-13
@grgia grgia added the Internal Requires API changes or must be handled by Expensify staff label Jul 14, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 14, 2023

Job added to Upwork: https://www.upwork.com/jobs/~013b675735e78d2be9

@melvin-bot
Copy link

melvin-bot bot commented Jul 14, 2023

Triggered auto assignment to Contributor Plus for review of internal employee PR - @sobitneupane (Internal)

@grgia
Copy link
Contributor Author

grgia commented Aug 31, 2023

cc @pac-guerreiro

@melvin-bot melvin-bot bot added the Overdue label Aug 31, 2023
@pac-guerreiro
Copy link
Contributor

Could you assign me to this @grgia ? 😄

@marcaaron
Copy link
Contributor

Similar to the issue for adding the Expensify Card page. I think we can get started on this, but ultimately HOLD merging the PR on having the Expensify Card page itself ready.

One way to rollout and get some early benefit from these changes would probably be something like:

  • Set up the routes and form with test data (but leave off the button and no need to call any API yet) - this way they are ready to go in the future
  • Have the Expensify Card page displaying the virtual card
  • Wait for API changes to get deployed (not sure if anyone has started on them)
  • Add the button to the Expensify Card page so the user can now navigate <- at this point we would also hook up the API and test the whole card request flow from E2E.

@marcaaron marcaaron changed the title [HOLD #22873][ECard Settings] - Add “Get physical card” button and necessary routes [ECard Settings] - Add “Get physical card” button and necessary routes Aug 31, 2023
@marcaaron marcaaron added Daily KSv2 and removed Monthly KSv2 labels Aug 31, 2023
@melvin-bot melvin-bot bot removed the Overdue label Aug 31, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 4, 2023

@sobitneupane, @pac-guerreiro Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot
Copy link

melvin-bot bot commented Sep 12, 2023

@sobitneupane, @pac-guerreiro 10 days overdue. I'm getting more depressed than Marvin.

@melvin-bot
Copy link

melvin-bot bot commented Sep 14, 2023

@sobitneupane, @pac-guerreiro 12 days overdue. Walking. Toward. The. Light...

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Sep 18, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 18, 2023

This issue has not been updated in over 14 days. @sobitneupane, @pac-guerreiro eroding to Weekly issue.

@melvin-bot melvin-bot bot removed the Overdue label Sep 18, 2023
@sobitneupane
Copy link
Contributor

@pac-guerreiro Is this issue on hold?

@pac-guerreiro
Copy link
Contributor

@sobitneupane

Yes it is, but I'm working on it 😄

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production labels Nov 23, 2023
@melvin-bot melvin-bot bot changed the title [ECARD] CRITICAL: Settings - Add “Get physical card” button and necessary routes [HOLD for payment 2023-11-30] [ECARD] CRITICAL: Settings - Add “Get physical card” button and necessary routes Nov 23, 2023
Copy link

melvin-bot bot commented Nov 23, 2023

Reviewing label has been removed, please complete the "BugZero Checklist".

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Nov 23, 2023
Copy link

melvin-bot bot commented Nov 23, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.2-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-11-30. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Nov 24, 2023
@allroundexperts
Copy link
Contributor

I would like to clarify here that the two deploy blockers linked here are not truly regressions whose penalty should apply.

The first blocker was not directly caused by the PR and we decided to dig into it in a follow up PR before merging this. More context here.

The second blocker was a very minor design in-consistency which went through the design review as well.

Given the above along with the size of the PR, I would like to request for penalty waiver if there is any. Let me know what you think @grgia!

@situchan
Copy link
Contributor

I also suggest no penalty. But you could review follow-up PRs caused by this PR, instead of letting other C+ review.
-#31702
-#31759

Before, we had discussion that if C+ also tasks responsible for regression issues, no penalty applies sometimes.
But if another C+ handle them, penalty applies because had to pay them for their review as well and that feels like throwing money out of window.
There's no official guideline of course.
cc: @mountiny

@grgia
Copy link
Contributor Author

grgia commented Nov 27, 2023

@situchan @allroundexperts I agree that the original C+ should handle follow-up issues from this PR. Sometimes, with the number of new issues created, especially deploy blockers that have an urgency applied, the auto assigner might be part of the confusion.

@allroundexperts I agree that the padding issue is not a serious regression and that any blockers related to the follow-up should be handled in the follow-up we decided on before merging.

For this specific issue, I think it's alright to waive the penalty- but for next time, I agree that any follow-ups should be reviewed by the original C+. Does that sound fair to everyone?

@allroundexperts
Copy link
Contributor

Yep, sounds good to me!

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Nov 30, 2023
Copy link

melvin-bot bot commented Dec 8, 2023

@allroundexperts, @grgia, @pac-guerreiro Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@allroundexperts
Copy link
Contributor

Ho @grgia!
I think my payment for the review is still pending here. Can you please write a payment summary?

Thanks

@mallenexpensify mallenexpensify added the Bug Something is broken. Auto assigns a BugZero manager. label Dec 13, 2023
Copy link

melvin-bot bot commented Dec 13, 2023

Triggered auto assignment to @abekkala (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

Copy link

melvin-bot bot commented Dec 13, 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

@mallenexpensify mallenexpensify changed the title [HOLD for payment 2023-11-30] [ECARD] CRITICAL: Settings - Add “Get physical card” button and necessary routes [HOLD for payment 2023-11-30][$500][ECARD] CRITICAL: Settings - Add “Get physical card” button and necessary routes Dec 13, 2023
Copy link

melvin-bot bot commented Dec 13, 2023

Upwork job price has been updated to $500

@mallenexpensify
Copy link
Contributor

Contributor+: @allroundexperts owed $500 via NewDot

@mallenexpensify
Copy link
Contributor

d;oh, sorry so messy here, should be all set/done @abekkala

@JmillsExpensify
Copy link

$500 payment approved for @allroundexperts based on comment above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 ECard Wave5-free-submitters Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review
Projects
No open projects
Development

No branches or pull requests