-
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
[HOLD for payment 2024-04-25] [HOLD for payment 2024-04-17] [Guided Setup] [$250] Sign up - Welcome modal window on Abracadabra page #39837
Comments
Triggered auto assignment to @cead22 ( |
👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:
|
@cead22 FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors |
I think it's a regression from #37733 |
Job added to Upwork: https://www.upwork.com/jobs/~017ea247a272b092bf |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav ( |
Coming from @mountiny on slack
|
I will try to help manage this in my morning if this wont be resolved by then. |
Confirmed that revert #37733 would fix this issue. Checking root cause |
ProposalPlease re-state the problem that we are trying to solve in this issue.Sign up - Welcome modal window on Abracadabra page What is the root cause of that problem?I think it is because of this change App/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts Lines 330 to 334 in 38ced97
=> It takes this condition is false when in the validate page => leads to show current onboarding modal in validate page in the next line App/src/libs/Navigation/AppNavigator/createCustomBottomTabNavigator/BottomTabBar.tsx Lines 47 to 49 in 38ced97
This is the log of
What changes do you think we should make in order to solve the problem?We probably revert this change App/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts Lines 330 to 334 in 38ced97
|
This change looks promising, but @hoangzinh @mountiny @adamgrzybowski do we know what would break if this block is reverted? |
@mananjadhav Okay I did some investigation and I think the solution is somehow similar but not exactly the same. The problem is that App/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts Lines 315 to 340 in 38ced97
Instead of reverting the lines mentioned above we need to modify the if Additionally it would be great to add a comment before if, maybe something like // We need to make sure that this if only handles states where we deeplink to the bottom tab directly
if(bottomTabNavigator && bottomTabNavigator.state) { |
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. |
I think your solution would solve for both @adamgrzybowski. But I haven't tested the solution yet. |
@adamgrzybowski could you explain a little bit about why do you think adjusting the condition above is better than reverting the changes? I'm worried that it potentially cause other regression bugs |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.62-17 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 2024-04-25. 🎊 For reference, here are some details about the assignees on this 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:
|
Whoops, removed that comment and holding off until the checklist is done and the payment date comes due. |
It's a DB issue so probably we don't have a BugZero Checklist |
@JmillsExpensify This was deployed on production and the original payout date was 2024-04-17. PR that caused the regression is #37733. It was already explained during the initial findings. We don't need a regression test here. This is ready for payout. |
@cead22, @mananjadhav, @hoangzinh, @miljakljajic, @mountiny Huh... This is 4 days overdue. Who can take care of this? |
@hoangzinh please accept the payment offer @mananjadhav payment summary: owed 250 USD |
Accepted. Thanks @miljakljajic |
$250 approved for @mananjadhav |
@cead22, @mananjadhav, @hoangzinh, @miljakljajic, @mountiny Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
I think we're good to close this one. @miljakljajic @mountiny |
yup process payment then we're good to close this issue. |
Which payment is missing? |
paid! |
Hi @miljakljajic I'm just checking Upwork and it appears that my payment for this issue hasn't been completed yet. Can you check again? Thank you. |
resolved |
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: 1.4.61-0
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/cases/view/2616817
Issue reported by: Applause - Internal Team
Action Performed:
Expected Result:
The abracadabra page is displayed
On original tab Concierge chat opens. The 'Welcome to Expensify' modal window appears
Actual Result:
The abracadabra page is displayed. The 'Welcome to Expensify' modal window appears on abracadabra page.
On original tab the skeleton loader displayed
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Bug6442285_1712582015558.bandicam_2024-04-08_16-05-31-752.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @miljakljajicThe text was updated successfully, but these errors were encountered: