-
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] [$1000] Add a mandatory route param to Navigation.goBack to prevent bugs #24291
Comments
Triggered auto assignment to @sophiepintoraetz ( |
Bug0 Triage Checklist (Main S/O)
|
Job added to Upwork: https://www.upwork.com/jobs/~01ee5e103f65c1a8fa |
Current assignee @sophiepintoraetz is eligible for the External assigner, not assigning anyone new. |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav ( |
Dupe of #22041? |
No this is a general improvement coming from Slack. Oh lol but I do see that we're having parallel conversations. |
We're trying to fix globally in that issue. |
Ok cool then I'll close this and link to the conversation we had. |
@neil-marcellini I think there is a subtle difference on what is being targetted there and what we are targetting here.
|
Ok that sounds different enough. I'll re-open this. |
I would love to work on this @neil-marcellini |
📣 @hanzalahsamana! 📣
|
@hanzalahsamana Please checkout our contributing guidelines to send out a proposal. |
Proposal Please re-state the problem that we are trying to solve in this issue. What is the root cause of that problem? What changes do you think we should make in order to solve the problem? What alternative solutions did you explore? (Optional) I appreciate your time considering this plan. If you want to talk more about it, I'm here to help. Best regards, |
@mananjadhav FYI I want to give first pick on this to @abdulrahuman5196 since we planned it together in Slack here. Abdul please post a proposal soon, and Manan please review his first. |
Sure. Will wait for @abdulrahuman5196 to post a proposal. |
ProposalPlease re-state the problem that we are trying to solve in this issue.Calling Navigation.goBack() in App without a fallback route will cause it to navigate to the HOME page if there is no back page to go. Like is the cases of refresh for eg. #22290. To fix this we have been adding fallback routes for different pages in different issue. But it will not stop a contributor from making the same mistake of just calling What is the root cause of that problem?Same as problem statement. What changes do you think we should make in order to solve the problem?We will make the fallback route param in navigation.goBack() mandatory - https://github.com/Expensify/App/blob/main/src/libs/Navigation/Navigation.js#L99 What alternative solutions did you explore? (Optional)This way of making the param mandatory was proposed as an alternative to adding a checklist item. We could also add an checklist item. |
I've bumped @neil-marcellini in new dot twice now - will escalate to slack after the holiday if we don't hear from him! |
I'm looking now sorry for the delay. |
📣 @mananjadhav Please request via NewDot manual requests for the Reviewer role ($1000) |
📣 @Prince-Mendiratta You have been assigned to this job! |
To be clear, @Prince-Mendiratta and @abdulrahuman5196 will be splitting the payment for this issue. |
PR is up! Kindly help review, @mananjadhav @abdulrahuman5196 @neil-marcellini :D |
@neil-marcellini The C+ for the PR was wrongly assigned, causing @mananjadhav to miss out on the PR, can you please assign him as the C+? Lots of PRs merging in, wouldn't want any regressions coming in due to delays. Thanks! |
I've assigned him! |
I am a bit unwell and I'll try to close this by tomorrow. |
Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:
On to the next one 🚀 |
|
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.
For reference, here are some details about the assignees on this issue:
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:
|
Payouts due: Issue Reporter: $250 N/A - Neil Eligible for 50% #urgency bonus? N |
We can treat this as a feature request, and not directly related to any specific feature here. We don't need a regression suite here as these can be covered in other features. We only changed by forcing an arg in the method call. |
$1,000 payment approved for @mananjadhav based on BZ summary. |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Problem
Calling
Navigation.goBack()
in App without a route can cause problems since it will navigate to the wrong page if the desired page to go back to is not the last page in the navigation history.Solution
Add a mandatory route param to Navigation.goBack so that the screen to navigate back to must be specified. Discussed in Slack here.
Platforms:
Which of our officially supported platforms is this issue occurring on?
All
Version Number:
Reproducible in staging?:
Reproducible in production?:
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
Expensify/Expensify Issue URL:
Issue reported by: @neil-marcellini
Slack conversation:
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: