-
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
HIGH [$750]: Clean up the payment options on Pay button in New Dot #36301
Comments
Triggered auto assignment to @joekaufmanexpensify ( |
Triggered auto assignment to @greg-schroeder ( |
Let's run with this issue! |
Let's hunt down proposals @joekaufmanexpensify 🦾 |
Unable to auto-create job on Upwork. The BZ team member should create it manually for this issue. |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @allroundexperts ( |
Job added to Upwork: https://www.upwork.com/jobs/~016cb0dade2c28979f |
Current assignee @allroundexperts is eligible for the External assigner, not assigning anyone new. |
Hmm yeah, there should still be the dropdown arrow portion of the split button to choose a different method if you want to. |
Agree with that. |
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. If a regression has occurred and you are the assigned CM follow the instructions here. If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future. |
@brandonhenry @allroundexperts are you able to take a look at those reports above? And we also have this one that popped up #48560 which I'm not sure if related or not... ideally I don't think we want/need to revert right now but if #37174 is the root cause maybe we do 🤷 |
FWIW, I think the Test/QA steps in #37174 are super light given the changes here that span a lot further than just pay options for iouReports. I'm not surprised this keeps getting reverted. Revisiting the OP to try and make sense of this one.
I assume those are the pay options for iouReports only, but it still suggests the button should be a button displayed with a menu for Then in the OP, there’s some additional logic changes for the stickiness..
This makes sense, as some pay button options on iouReports aren’t applicable or available on expenseReports on workspace expenses (i.e PBA or Debit Card). Your preferred payment method for each of those report types can conceivably be different. Did this get tested and implemented in the PR, or is it outstanding?
I think this last line in that bullet doesn't make sense though. It also contradicts the next bullet down, so let's move on to that bullet.
This sounds like something we are doing already by using the VBBA set in the workspace settings when you’re the reimburser viewing an expenseReport that needs reimbursing. You can choose to
Same here, doesn't this come out of the box with setting a VBBA to reimburse from in the workspace settings? Overall, it seems like this has got a bit wonky in implementation perhaps. I'm leaning towards reverting and revisiting this with a clearer picture of the scope of implementation. |
Agree with Tom |
@trjExpensify I agree.. and I also brought up in the PR the many weird discrepancies.. @allroundexperts was reviewer here, do you have thoughts on how to proceed? I mentioned here during PR how I was confused on the number of options needed but you wanted us to align on making this look exactly like staging (prior to merge). There's a very good chance our QA missed things here considering the wide encompassing nature, but my knowledge is limited and I don't think my QA along would have caught all the areas. That said, can we agree on a plan to get this implemented exactly how the design team would like, in all cases? Happy to open a new PR and own that, but would like further guidance on every workflow this touches |
|
Maybe a quick design doc could even make sense? The amount of discussion on this issue about implementation, and the slack threads linked out to already further discussing kinda shows this isn't really a straightforward change. It might benefit from making sure we are holistically considering all angles of the solution. |
100% |
Going to bring next steps to slack very soon. |
Okay, we discussed this internally and here's where we landed. We're going to:
LMK if anyone has questions on that. Otherwise will tackle the payment steps later today! |
Payment summary here:
|
@brandonhenry offer for $750 sent via Upwork |
@allroundexperts you're good to request payment here |
@joekaufmanexpensify thank you!! |
@brandonhenry $750 sent and contract ended! |
Upwork job closed. |
Closing this one out as all that's left is for @allroundexperts to request payment in Newdot. We'll be continuing discussion on the longer term solution for this in Slack! |
$750 approved for @allroundexperts |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: v1.4.39-7
Reproducible in staging?: Y
Reproducible in production?: Y
Expensify/Expensify Issue URL: #33967
Issue reported by: @anmurali
Slack conversation: NA -
Action Performed:
Expected Result:
The same options should be presented when clicking "Pay with Expensify" or the “⌄” down chevron beside it.
This is the new design we want to implement lifted from #33967:
Actual Result:
Two different sets of options are presented to the user:
A:
B:
Workaround:
None
Platforms:
Which of our officially supported platforms is this issue occurring on?
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: