Skip to content
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-02-02] Manage members: Opacity reduces pressing on the workspace admin #13769

Closed
2 tasks
kavimuru opened this issue Dec 21, 2022 · 32 comments
Closed
2 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Internal Requires API changes or must be handled by Expensify staff Weekly KSv2

Comments

@kavimuru
Copy link

kavimuru commented Dec 21, 2022

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:

  1. Open Settings.
  2. Go to Workspaces.
  3. Create or open any existing workspace.
  4. Press on admin.

Expected Result:

Shouldn’t reduce opacity bcz its not a selectable item.

Actual Result:

Opacity reducing like normal active item.

Workaround:

unknown

Platforms:

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

  • Windows OS / Chrome / Safari
  • MacOS / Desktop

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

Recording.1135.mp4
opacity.mov

Expensify/Expensify Issue URL:
Issue reported by: @jatinsonijs
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1671602565821529

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01abccdea17bf0637e
  • Upwork Job ID: 1608221055756103680
  • Last Price Increase: 2022-12-28
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 21, 2022
@melvin-bot melvin-bot bot locked and limited conversation to collaborators Dec 21, 2022
@sketchydroide
Copy link
Contributor

I would say that this might be a bug, but it's weird that you can select it as well.
Maybe we shoudl not even show the ceckbox for the admin, right?
Will bring this to slack to get some opinions.

@sketchydroide sketchydroide self-assigned this Dec 22, 2022
@melvin-bot melvin-bot bot added the Overdue label Dec 26, 2022
@sketchydroide
Copy link
Contributor

I will post on the ope source channel about this, but I would adivise either not show the user, or not be selectable (this includes hiding the checkbox for this users)

@melvin-bot melvin-bot bot removed the Overdue label Dec 27, 2022
@sketchydroide
Copy link
Contributor

sketchydroide commented Dec 28, 2022

So it was decided that just disabling the opacity for the admins was enough as we already show an error on mobile and a tip on desktop and web.
Will do this today.

@tjferriss
Copy link
Contributor

@sketchydroide sounds like you're going to make the fix so I'm going to add the internal label.

@tjferriss tjferriss added the Internal Requires API changes or must be handled by Expensify staff label Dec 28, 2022
@sketchydroide
Copy link
Contributor

it's a bit more complex than that actually, but for now that sounds good.
There is a bit more context on this slack thread
But basically I raised some concerns about the UX being quite different on mobile and desktop, and there might be some level of planning involved.

@sketchydroide sketchydroide added the Planning Changes still in the thought process label Dec 29, 2022
@sketchydroide
Copy link
Contributor

@JmillsExpensify do you think this should still be a bug atm? I feel there is a bug, still, but kinda maybe a feature as well, as we might use similar patterns elsewhere or in the future.

CCing @Puneet-here and @luacmartins as you might have some input on this

@JmillsExpensify
Copy link

I think it is a bug, but I think it's one of those bugs where solving it requires a broader plan, so we need a process to figure out how to handle these. Like reliable notifications or sequence numbers, they don't fit in WAQ. Perhaps for now we remove any issue with the planning label from the WAQ report, just like we're doing for holds.

@melvin-bot melvin-bot bot added the Overdue label Jan 2, 2023
@sketchydroide
Copy link
Contributor

Perhaps for now we remove any issue with the planning label from the WAQ report

yep I think that would make sense, specially since they take way more time to be finished.

@luacmartins I think you should be back from OOO let me know your thoughts on this (not sure if you know anything, but your knowledge of forms would probably help)

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jan 2, 2023
@luacmartins
Copy link
Contributor

Left a comment in the thread

@sketchydroide
Copy link
Contributor

Your input on the thread sounds good, I think I can do something about it.
Just for making it easier here is @luacmartins suggestion

So for consistency, I think that we should allow the user to select any checkbox they want, but show an error when they try to submit it. That will remove the growl and UI/UX inconsistencies.

I will updat this GH to better exlpain the issue and the solution

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jan 5, 2023
@melvin-bot melvin-bot bot unlocked this conversation Jan 9, 2023
@sketchydroide
Copy link
Contributor

I might be able to work on it this week if not I'll let someone else pick it up as I'm going OOO after tomorrow for a week

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jan 9, 2023
@tjferriss
Copy link
Contributor

@sketchydroide should we look for a volunteer in Slack or could we add the First Pick label?

@melvin-bot melvin-bot bot removed the Overdue label Jan 11, 2023
@luacmartins
Copy link
Contributor

I see that @sketchydroide is ooo. I'll take this over until he's back.

@luacmartins luacmartins self-assigned this Jan 16, 2023
@luacmartins
Copy link
Contributor

Gonna work on this tomorrow!

@melvin-bot melvin-bot bot removed the Overdue label Jan 16, 2023
@luacmartins luacmartins removed the Planning Changes still in the thought process label Jan 17, 2023
@melvin-bot melvin-bot bot added the Reviewing Has a PR in review label Jan 17, 2023
@luacmartins
Copy link
Contributor

luacmartins commented Jan 18, 2023

PR up

@sketchydroide
Copy link
Contributor

Thanks for picking this up @luacmartins

@luacmartins
Copy link
Contributor

PR merged! Waiting on deploys!

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Jan 26, 2023
@melvin-bot melvin-bot bot changed the title Manage members: Opacity reduces pressing on the workspace admin [HOLD for payment 2023-02-02] Manage members: Opacity reduces pressing on the workspace admin Jan 26, 2023
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jan 26, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jan 26, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jan 26, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.59-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-02-02. 🎊

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
Copy link

melvin-bot bot commented Jan 26, 2023

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:

  • [@rushatgabhane / @sketchydroide / @luacmartins] The PR that introduced the bug has been identified. Link to the PR:
  • [@rushatgabhane / @sketchydroide / @luacmartins] 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:
  • [@rushatgabhane / @sketchydroide / @luacmartins] 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:
  • [@tjferriss] 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:

@rushatgabhane
Copy link
Member

This wasn't really a bug. It's an UX preference so no need to do the checklist

@jatinsonijs
Copy link
Contributor

@tjferriss Gentle reminder. Regression period is passed (2 feb).

@melvin-bot melvin-bot bot added the Overdue label Feb 6, 2023
@luacmartins
Copy link
Contributor

@tjferriss bump for payment!

@melvin-bot melvin-bot bot removed the Overdue label Feb 6, 2023
@tjferriss
Copy link
Contributor

tjferriss commented Feb 6, 2023

Sorry for the delay. The original Upworks post was closed so I've created a new one here:https://www.upwork.com/jobs/~0197b675e1adf982dc

@jatinsonijs can you please apply here?

@Johannes-Harald

This comment was marked as resolved.

@tjferriss
Copy link
Contributor

The offer is pending. Once you accept @jatinsonijs, I can make payment.

@jatinsonijs
Copy link
Contributor

Accepted @tjferriss

@tjferriss
Copy link
Contributor

The payment has been made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Internal Requires API changes or must be handled by Expensify staff Weekly KSv2
Projects
None yet
Development

No branches or pull requests

8 participants