[HOLD for payment 2022-12-22][$1000] Workspace - general settings - `Save` Button is not at the bottom

kavimuru commented 1 year ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Action Performed:

Open the app
Go to settings > Workspaces
Open a workspace
Go to workspace settings

Expected Result:

The button should be at the bottom

Actual Result :

The button is not at the bottom

Workaround:

unknown

Platform:

Where is this issue occurring?

Web
iOS
Android
Desktop App
Mobile Web

Version Number: 1.2.37-2 Reproducible in staging?: y Reproducible in production?: n Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos:

Expensify/Expensify Issue URL:

Issue reported by: @daraksha-dk

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1670351071306529

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~014a8b2e372f1048cf
Upwork Job ID: 1602417572384657408
Last Price Increase: 2022-12-12

melvin-bot[bot] commented 1 year ago

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

daraksha-dk commented 1 year ago

Proposal

styles.flex1 was removed from below in https://github.com/Expensify/App/pull/12848 (cc: @tgolen ) https://github.com/Expensify/App/blob/ed3e2dc507e0f02a30b22a012412fbecc0be2290/src/components/FormAlertWithSubmitButton.js#L56

We just have to pass styles.flex1 below so it will only affect Form https://github.com/Expensify/App/blob/ed3e2dc507e0f02a30b22a012412fbecc0be2290/src/components/Form.js#L297

tgolen commented 1 year ago

OK, please make sure that you test the sign-in flows to ensure buttons are all placed properly. If I recall, the reason I removed that was because it was causing a spacing issue on the "resend validation link" form.

daraksha-dk commented 1 year ago

@tgolen, yeah I looked at the PR and re-adding flex1 below was causing spacing issues https://github.com/Expensify/App/blob/ed3e2dc507e0f02a30b22a012412fbecc0be2290/src/components/FormAlertWithSubmitButton.js#L56

that's why I found another solution, which is to pass the flex1 to FormAlertWithSubmitButton using by using its containerStyles prop in Form so that it doesn't affect other places. We just have to pass flex1 below https://github.com/Expensify/App/blob/ed3e2dc507e0f02a30b22a012412fbecc0be2290/src/components/Form.js#L297

chiragsalian commented 1 year ago

Proposal LGTM

davidcardoza commented 1 year ago

Applying the external label for review.

melvin-bot[bot] commented 1 year ago

Current assignee @davidcardoza is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~014a8b2e372f1048cf

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav (External)

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @chiragsalian (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

davidcardoza commented 1 year ago

Job posted - https://www.upwork.com/jobs/~01fa021a1ff96c59c0

lordthien commented 1 year ago

Proposal: In file: App/src/components/Form.js. I add styles.flex1 into containerStyles of FormAlertWithSubmitButton

diff --git a/src/components/Form.js b/src/components/Form.js
index 72ee4afc6b..3c41357161 100644
--- a/src/components/Form.js
+++ b/src/components/Form.js
@@ -294,7 +294,7 @@ class Form extends React.Component {
                                     focusInput.focus();
                                 }
                             }}
-                            containerStyles={[styles.mh0, styles.mt5]}
+                            containerStyles={[styles.mh0, styles.mt5, styles.flex1]}
                             enabledWhenOffline={this.props.enabledWhenOffline}
                             isDangerousAction={this.props.isDangerousAction}
                         />

Simulator Screen Shot - iPhone 13 Pro Max - 2022-12-13 at 10 50

mananjadhav commented 1 year ago

I just went through the proposals to review and was going to accept @daraksha-dk's proposal but then I saw the PR is already merged.

@chiragsalian do you need a second testing from my side here?

melvin-bot[bot] commented 1 year ago

📣 @daraksha-dk You have been assigned to this job by @chiragsalian! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

chiragsalian commented 1 year ago

Nope, we're good here. We moved a bit fast since deploy blockers are considered hourly. QA is testing this atm.

chiragsalian commented 1 year ago

QA tested this and its a pass, removing deploy blocker label

melvin-bot[bot] commented 1 year ago

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

https://github.com/Expensify/App/pull/13530

If no regressions arise, payment will be issued on 2022-12-20. :confetti_ball:

After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:

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

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

Merged PR within 3 business days of assignment - 50% bonus
Merged PR more than 9 business days after assignment - 50% penalty

melvin-bot[bot] commented 1 year ago

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:

[x] [@chiragsalian] The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/App/pull/12848
[x] [@chiragsalian] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/12848#issuecomment-1352350010
[x] [@chiragsalian] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A minor visual bug.
[x] [chirag] ~A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here: ~ Just a visual improvement. I'm not sure if this was ever associated to something else.

melvin-bot[bot] commented 1 year ago

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

https://github.com/Expensify/App/pull/13530

If no regressions arise, payment will be issued on 2022-12-22. :confetti_ball:

After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:

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

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

Merged PR within 3 business days of assignment - 50% bonus
Merged PR more than 9 business days after assignment - 50% penalty

melvin-bot[bot] commented 1 year ago

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:

[ ] [@chiragsalian] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@chiragsalian] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
[ ] [@chiragsalian] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
[x] [@davidcardoza] A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here:

daraksha-dk commented 1 year ago

@davidcardoza, I have applied to the job. Just a note - The job price is $1000 but it's eligible for $1750 (Reporting Bonus + Fix + 50% bonus )

davidcardoza commented 1 year ago

Sounds good, I paid out $1750

daraksha-dk commented 1 year ago

@davidcardoza, I haven't received the offer. Can you check please?

davidcardoza commented 1 year ago

@daraksha-dk I sent the invite again. Did you receive it?

mallenexpensify commented 1 year ago

@daraksha-dk , I just hired you, please accept https://www.upwork.com/jobs/~014a8b2e372f1048cf The bonus will be added, $1750 will be the total.
I've assigned to issue payment too, I think David might (or should) out of office :)

daraksha-dk commented 1 year ago

Thanks @mallenexpensify - I have accepted the offer on Upwork

mallenexpensify commented 1 year ago

Paid @daraksha-dk $1750. @chiragsalian can you fill out the BZ checklist above plz, Doza already did the TestRail update https://github.com/Expensify/App/issues/13488#issuecomment-1353792630

chiragsalian commented 1 year ago

huh, i already filled it out here - https://github.com/Expensify/App/issues/13488#issuecomment-1349867184. The second post looks like a melvin dupe so I'm ignoring it.

mallenexpensify commented 1 year ago

oh... sweet. Sorry I missed that @chiragsalian , closing meow

Expensify / App