-
Notifications
You must be signed in to change notification settings - Fork 3k
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-10-02] [HOLD for payment 2023-09-29] IOU - Next button disabled when requesting a new distance IOU offline #27895
Comments
Triggered auto assignment to @kevinksullivan ( |
Bug0 Triage Checklist (Main S/O)
|
Ah shoot yeah you're right. Thanks for finding that. @paultsimura would you please put up a quick fix? This is a deploy blocker. |
👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open
|
Triggered auto assignment to @youssef-lr ( |
I'll take care of this one since it's very early for Paul. |
@neil-marcellini thank you for taking care of the PR. I still do not understand how my original PR affected the isOffline logic – you can see that my PR's diff with latest main has not changed the Also, from this discussion with you, this behavior seemed expected. Could you please point me to the commit where I overwritten the change that allowed requesting in offline mode? |
I agree with @paultsimura, this behaviour was noted in the PR and considered expected as @paultsimura pointed out. We didn't modify anything around the |
@paultsimura yes, it's not a regression from your PR. What I mean here is, in the middle of your ongoing PR, there is another PR merged that disables the button when offline, so when you merge with the main, you will get that behavior. However, this issue is only created after your PR because your PR expects the distance request to work while offline. |
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.72-11 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-09-29. 🎊 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.
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.73-1 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-10-02. 🎊 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.
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
@kevinksullivan, @neil-marcellini Whoops! This issue is 2 days overdue. Let's get this updated quick! |
Regression Test ProposalI think we should have a regression test since it's simple to test and easy to break.
Do we agree 👍 or 👎 |
@Ollyws sent you an offer for PR review |
@kevinksullivan Thanks but I didn't review this one. |
@Ollyws you reviewed the PR and filled out the checklist here |
@neil-marcellini Oh I see, so this is being counted as a regression of #26836, with the accompanying 50% penalty then? |
@kevinksullivan, @neil-marcellini Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
Yes I think so. Let's get the payment wrapped up here please. |
Hey @neil-marcellini could you please check these comments? #27895 (comment) I still don't think this was a regression, since enabling the offline flow was not on the plate of the original issue and was even clearly discussed in the PR. However, if I'm referencing the wrong issue – please just disregard this message. |
@neil-marcellini Ah we have already been paid for the original issue. Although I'm happy to go with whatever decision the team makes, I was fully aware of this peice of code as it had to be reverted to test the PR atall, it's just I understood this comment of yours to mean that this was essentially a closed flow until a later date and we were to leave it disabled. So a communication error, rather than a regression, in my opinion. |
Ok sorry about that and thanks for explaining. I see that the "regression" came from this PR. Also as you mentioned it wasn't really a regression because it was expected at that time. It's all very confusing so let's not penalize any payments.
Ah ok great, then I'm closing this. |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Issue found when executing PR #26836
Action Performed:
Expected Result:
User should be able to click Next and there the fields amount and distance are filled as "TBD"
Actual Result:
User should be able to click Next and there the fields amount and distance are filled as "TBD"
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.72-6
Reproducible in staging?: Yes
Reproducible in production?: Yes
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
Bug6207748_Recording__328.mp4
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:
View all open jobs on GitHub
The text was updated successfully, but these errors were encountered: