[HOLD for payment 2023-07-12] [$1000] Auth DeletePolicy error is misaligned

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
Open workspaces
Select any workspace and delete it

Expected Result:

If Auth DeletePolicy error is triggered, it should be aligned with workspace avatars as we normally do for other errors

Actual Result:

Auth DeletePolicy error is not aligned with workspace avatars

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

Which of our officially supported platforms is this issue occurring on?

[x] Android / native
[x] Android / Chrome
[x] iOS / native
[x] iOS / Safari
[x] MacOS / Chrome / Safari
[x] MacOS / Desktop

Version Number: 1.3.29-0 Reproducible in staging?: y Reproducible in production?: y 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 image (3)

https://github.com/Expensify/App/assets/43996225/c4f305c9-408d-4722-8e41-0bf4abee79e5

Expensify/Expensify Issue URL: Issue reported by: @dhanashree-sawant Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1686578487180049

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01ae4f5bbe870cdf89
Upwork Job ID: 1671580913028014080
Last Price Increase: 2023-06-21

ahmedGaber93 commented 1 year ago

Proposal

Auth DeletePolicy error is misaligned

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

The error padding left in not the same as the item padding left. Error padding left: https://github.com/Expensify/App/blob/332d2c96e1f0b4bbcdd85859868b6d7b3047d76a/src/styles/styles.js#L2832-L2835

Item padding left: https://github.com/Expensify/App/blob/332d2c96e1f0b4bbcdd85859868b6d7b3047d76a/src/styles/styles.js#L1299

What changes do you think we should make in order to solve the problem?

Apply the same padding left for both Error and Item

 menuItemErrorPadding: { 
     paddingLeft: 20, 
     paddingRight: 20, 
 },

Screenshot 2023-06-19 at 11 25 26 PM

What alternative solutions did you explore? (Optional)

N/A

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

[ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
[ ] This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
[ ] This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
[ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

jeet-dhandha commented 1 year ago

I would like to suggest to not get this aligned as this looks more like a error message now rather then when aligned, where it will look more like a workspace.

dukenv0307 commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

The error dot is misaligned

What is the root cause of that problem?

This is the currently padding/ margin of dot error in all pages

Padding Horizontal 24px

Margin Horizontal 20px

no padding
Padding Horizontal 20px
Padding left 44px, padding right 20px

What changes do you think we should make in order to solve the problem?

We need to confirm the padding/margin for each above page. This is my suggestion

Padding horizontal 20px

https://github.com/Expensify/App/blob/7c04c2f3f10df403f97fecb3b18dd81478003a31/src/pages/workspace/WorkspaceInitialPage.js#L197

update styles.ph6 to styles.ph5

Look good
Padding horizontal 20px https://github.com/Expensify/App/blob/7c04c2f3f10df403f97fecb3b18dd81478003a31/src/pages/settings/Profile/Contacts/ContactMethodsPage.js#L96

Add this props

errorRowStyles={[styles.ph5]}

Look good
Padding horizontal 20px https://github.com/Expensify/App/blob/7c04c2f3f10df403f97fecb3b18dd81478003a31/src/styles/styles.js#L2835 update to paddinghorizontal: 20

What alternative solutions did you explore? (Optional)

@Beamanator Could you help to confirm the correct padding margin in above places

Beamanator commented 1 year ago

Maybe @shawnborton can help confirm how we want those to look - and @dukenv0307 it looks like you forced errors to appear in Onyx, right? Maybe you can add some dummy text to each of those screenshots too?

dukenv0307 commented 1 year ago

Sorry for my bump mistake @shawnborton Please help to confirm

shawnborton commented 1 year ago

I'm confused, why are we not showing an error message next to the red dot?

dukenv0307 commented 1 year ago

@shawnborton Because this bug focuses on the error being misaligned so I am forcing errors to appear red dot in code without a message. I will update to include message error for more clear

shawnborton commented 1 year ago

Thank you!

dukenv0307 commented 1 year ago

@shawnborton I just updated https://github.com/Expensify/App/issues/21143#issuecomment-1600106970 Could you help to check? Thanks

shawnborton commented 1 year ago

I think the cleanest thing to do might be to do just Padding Horizontal 20px from your ideas. But I guess it depends, if that error message is already wrapped inside a view that has 20px padding, then we wouldn't need any extra. Let me know if that makes sense.

dukenv0307 commented 1 year ago

padding-left: 30px, padding-right: 20px

@shawnborton In the number 1, padding-left should be 30px instead of 20px to aligned with the above icon. What do you think about this?

shawnborton commented 1 year ago

I disagree with that. I think we're going to run into too many different cases of trying to align the red dot up with the icon above it - for example, there are different sizes when there is an avatar from a chat message, vs an icon from an option row, etc. So I think the cleanest thing to do is follow the padding that the view has if it's say 20px, and if there is no view padding, then we use 20px.

dukenv0307 commented 1 year ago

Update proposal with the lastest discussion

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01ae4f5bbe870cdf89

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

Santhosh-Sellavel commented 1 year ago

I think the first proposal @ahmedGaber93's is straightforward so let's hire them, other proposals are redundant. Thanks, everyone for the proposals and discussion!

@wahaha999 About your alternative proposal, I really don't see why we need it. If your proposal solves or improves any issues and multiple components that are not being reported please start a thread in the Expensify Slack channel with a list of Problems and how your solution addresses them, thanks!

C+ Reviewed 🎀 👀 🎀

melvin-bot[bot] commented 1 year ago

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

dukenv0307 commented 1 year ago

@Santhosh-Sellavel A small question, Why do you think we shouldn't fix here https://github.com/Expensify/App/blob/7c04c2f3f10df403f97fecb3b18dd81478003a31/src/pages/workspace/WorkspaceInitialPage.js#L197 update to styles.ph5 for consistency

dukenv0307 commented 1 year ago

We can display error on WorkspaceInitialPage be these step:

Do to any workspace (ID: 123456)
Delete this workspace
GO to again this workspace after removing it by accessing this URL: /workspace/123456

It will display like this with wrong padding

ahmedGaber93 commented 1 year ago

@Santhosh-Sellavel we can also fix any similar reported/non-reported issue not included in this issue report in one PR with same My proposal concept. My proposal concept is:

Apply the same visual padding/margin for both Error and Item above it.

And any other similar issues like this issue #20420 and my proposal there and others, we can cover it in one PR.

Thanks!

Santhosh-Sellavel commented 1 year ago

@ahmedGaber93 @dukenv0307 Sorry for the confusion here, I think both are valid proposals in their own way. I'm not sure whom to pick here. Are you guys interested in working together on this one and sharing the job fee, so we can get this done sooner?

cc: @madmax330 What do you say?

I'll let you make the final call here, thanks!

ahmedGaber93 commented 1 year ago

Are you guys interested in working together on this one and sharing the job fee

@Santhosh-Sellavel for me, no problem, I agree.

dukenv0307 commented 1 year ago

Great, I am happy to work with @ahmedGaber93

madmax330 commented 1 year ago

Alright I will assign you both. Thanksk @Santhosh-Sellavel

melvin-bot[bot] commented 1 year ago

📣 @dukenv0307 You have been assigned to this job by @madmax330! 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 📖

melvin-bot[bot] commented 1 year ago

📣 @ahmedGaber93 You have been assigned to this job by @madmax330! 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 📖

Santhosh-Sellavel commented 1 year ago

Thanks @madmax330!

@dukenv0307 & @ahmedGaber93 One of you should implement and the other one should help to review/test fill the checklist. Please decide on that and get started soon, thanks!

ahmedGaber93 commented 1 year ago

Thanks @Santhosh-Sellavel I will create the PR soon, and ask @dukenv0307 for review and test

dukenv0307 commented 1 year ago

@ahmedGaber93 Ping me when PR is ready

ahmedGaber93 commented 1 year ago

We can display error on WorkspaceInitialPage be these step:

Do to any workspace (ID: 123456)

Delete this workspace

GO to again this workspace after removing it by accessing this URL: /workspace/123456

@dukenv0307 I can't show the error by these steps, it displays "Hmm... it's not here".

dukenv0307 commented 1 year ago

@ahmedGaber93 We need to make that there is an error while deleting workspace in step 2

ahmedGaber93 commented 1 year ago

@Santhosh-Sellavel @dukenv0307 PR is ready for review.

maddylewis commented 1 year ago

Making note of payments just for my own organization:

[x] Reporting bonus - @dhanashree-sawant ($250)
[x] Contributor - @dukenv0307 ($750)
[x] Contributor - @ahmedGaber93 ($750)
[x] C+ - @Santhosh-Sellavel ($1000)

dhanashree-sawant commented 1 year ago

Hi @maddylewis, I think you meant @dhanashree-sawant for reporting bonus 😅

melvin-bot[bot] commented 1 year ago

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

when @ahmedGaber93 got assigned: 2023-06-26 10:32:23 Z
when the PR got merged: 2023-07-03 13:10:02 UTC
days elapsed: 5

On to the next one 🚀

Santhosh-Sellavel commented 1 year ago

@madmax330 I think this should be considered for a speed bonus, as it's been ready for merge 4 days ago. Thanks!

cc: @maddylewis

maddylewis commented 1 year ago

@dhanashree-sawant - ah, sorry about that! https://github.com/Expensify/App/issues/21143#issuecomment-1609962297

updated!!

maddylewis commented 1 year ago

@Santhosh-Sellavel - i updated the total payouts for the contributors to $750 - https://github.com/Expensify/App/issues/21143#issuecomment-1609939334

if that works, I'll get the payments sent. thanks!

melvin-bot[bot] commented 1 year ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 1 year ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.36-5 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/21662

If no regressions arise, payment will be issued on 2023-07-12. :confetti_ball:

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.

[x] External issue reporter
[x] Contributor that fixed the issue
[x] 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:

[ ] [@Santhosh-Sellavel] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@Santhosh-Sellavel] 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:
[ ] [@Santhosh-Sellavel] 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:
[ ] [@Santhosh-Sellavel] Determine if we should create a regression test for this bug.
[ ] [@Santhosh-Sellavel] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[ ] [@maddylewis] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

maddylewis commented 1 year ago

payment due soon / switching to daily

maddylewis commented 1 year ago

ill process payment tomorrow!

maddylewis commented 1 year ago

@Santhosh-Sellavel - let me know the status of the BZ checklist https://github.com/Expensify/App/issues/21143#issuecomment-1622148429

maddylewis commented 1 year ago

offers sent

Santhosh-Sellavel commented 1 year ago

@maddylewis I will collect via ND, so cancel my offer. Thanks!

Santhosh-Sellavel commented 1 year ago

@maddylewis Bonus for me is not included here. Please update so I can proceed with the ND request!

Expensify / App