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

[Community Permissions] Update Is allowed, In and Hide permission logic according to the new design #9211

Merged
merged 4 commits into from
Jan 26, 2023

Conversation

micieslak
Copy link
Member

@micieslak micieslak commented Jan 20, 2023

What does the PR do

  • Disable granting admin permission for non-owners
  • Simplify InDropdown popup (selecting only channels, no add channel button)
  • Setting icon/default values for 'In' section depending on chosen permission
  • Adjust behavior of 'Hide permission' switch depending on selected permission

Closes #9050

Needed alignments for StatusSwitch are excluded to a separate ticket: #9212

Affected areas

CommunityNewPermissionView and releted components

Screenshot of functionality (including design for comparison)

  • I've checked the design and this PR matches it
Kazam_screencast_00104.webm

@status-im-auto
Copy link
Member

status-im-auto commented Jan 20, 2023

Jenkins Builds

Click to see older builds (20)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ c8c2043 #1 2023-01-20 11:47:06 ~4 min imports 📄log
✔️ c8c2043 #1 2023-01-20 11:48:39 ~6 min tests-nim 📄log
✔️ c8c2043 #1 2023-01-20 11:53:46 ~11 min linux 📦tgz
✔️ c8c2043 #1 2023-01-20 11:59:12 ~16 min macos 🍎dmg
✔️ c8c2043 #1 2023-01-20 12:11:37 ~29 min windows 💿exe
✖️ c8c2043 #1 2023-01-20 12:13:48 ~31 min linux-e2e 📄log
✔️ 5537a1d #2 2023-01-20 22:58:55 ~4 min imports 📄log
✔️ 5537a1d #2 2023-01-20 22:59:33 ~4 min linux-cpp 📄log
✔️ 5537a1d #2 2023-01-20 22:59:58 ~5 min tests-nim 📄log
✔️ 5537a1d #2 2023-01-20 23:03:46 ~9 min macos 🍎dmg
✔️ 5537a1d #2 2023-01-20 23:06:19 ~11 min linux 📦tgz
✖️ 5537a1d #2 2023-01-20 23:22:08 ~27 min linux-e2e 📄log
✔️ 5537a1d #2 2023-01-20 23:22:49 ~28 min windows 💿exe
✔️ 8e89720 #3 2023-01-24 14:24:06 ~4 min linux-cpp 📄log
✔️ 8e89720 #3 2023-01-24 14:24:21 ~5 min tests-nim 📄log
✔️ 8e89720 #3 2023-01-24 14:24:38 ~5 min imports 📄log
✔️ 8e89720 #3 2023-01-24 14:26:49 ~7 min macos 🍎dmg
✔️ 8e89720 #3 2023-01-24 14:30:19 ~11 min linux 📦tgz
✔️ 8e89720 #3 2023-01-24 14:44:58 ~25 min windows 💿exe
✖️ 8e89720 #3 2023-01-24 14:45:20 ~26 min linux-e2e 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ c3700c4 #4 2023-01-24 15:01:53 ~5 min imports 📄log
✔️ c3700c4 #4 2023-01-24 15:03:41 ~7 min macos 🍎dmg
✔️ c3700c4 #4 2023-01-24 15:04:39 ~7 min tests-nim 📄log
✔️ c3700c4 #4 2023-01-24 15:09:07 ~12 min linux 📦tgz
✖️ c3700c4 #4 2023-01-24 15:21:11 ~24 min linux-e2e 📄log
✔️ c3700c4 #4 2023-01-24 15:21:53 ~25 min windows 💿exe
✖️ c3700c4 #5 2023-01-24 21:21:20 ~26 min linux-e2e 📄log
✖️ c3700c4 #6 2023-01-24 22:54:10 ~26 min linux-e2e 📄log
✖️ c3700c4 #7 2023-01-25 09:07:20 ~27 min linux-e2e 📄log
✖️ c3700c4 #8 2023-01-25 10:45:48 ~29 min linux-e2e 📄log
✖️ c3700c4 #9 2023-01-25 14:05:56 ~22 min linux-e2e 📄log
✖️ c3700c4 #10 2023-01-25 14:34:36 ~27 min linux-e2e 📄log
✖️ c3700c4 #11 2023-01-25 20:21:43 ~28 min linux-e2e 📄log
✖️ c3700c4 #14 2023-01-26 08:13:41 ~7 min linux-e2e 📄log
✖️ c3700c4 #15 2023-01-26 09:46:03 ~23 min linux-e2e 📄log
✔️ ea7510b #5 2023-01-26 09:52:37 ~4 min linux-cpp 📄log
✔️ ea7510b #5 2023-01-26 09:53:30 ~5 min imports 📄log
✔️ ea7510b #5 2023-01-26 09:55:33 ~7 min macos 🍎dmg
✔️ ea7510b #5 2023-01-26 09:56:05 ~7 min tests-nim 📄log
✔️ ea7510b #5 2023-01-26 10:01:43 ~13 min linux 📦tgz
✔️ ea7510b #16 2023-01-26 10:05:33 ~17 min linux-e2e 📄log
✔️ ea7510b #5 2023-01-26 10:13:41 ~25 min windows 💿exe

Copy link
Member

@caybro caybro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just some suggestions

Copy link
Contributor

@alexjba alexjba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I've also tested the changes and it works nicely.

Copy link
Contributor

@noeliaSD noeliaSD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job!
Some minor comments added related to your changes:

  • The Edit flow is not correct. I.e: When you select Become member, then create the new permission, then go to Edit. The In selection should be the community one. Now it appears the option of selecting channels:
    Screenshot 2023-01-23 at 12 49 38

Other comments not related to this pr but it would be nice if you also fix them as part of it:

  1. Read is View only in the design (PermissionsDropdown.qml)
  2. Is allowwed to icons are black instead of blue when they are selected:
    Screenshot 2023-01-23 at 12 44 50

@noeliaSD
Copy link
Contributor

One other comment... If I remember correctly, the last decision made regarding the disabled but ON switch when admin is selected for the Hide Permission switch was... not to show the switch, right?

@micieslak
Copy link
Member Author

Thank you @noeliaSD for your comments!

  • The Edit flow is not correct. I.e: When you select Become member, then create the new permission, then go to Edit. The In selection should be the community one. Now it appears the option of selecting channels:

Could we address it in this dedicated task: #8855 ? I think it will be easier to do it completely there.

Other comments not related to this pr but it would be nice if you also fix them as part of it:

  1. Read is View only in the design (PermissionsDropdown.qml)

Done

  1. Is allowwed to icons are black instead of blue when they are selected:
    Screenshot 2023-01-23 at 12 44 50

Yeah, I noticed that and tried to fix, but it requires a closer look imo. From what I saw it may need some more complex changes to have StatusItemSelector working properly with various sources of image icons. So I would opt for addressing it separately.

@micieslak
Copy link
Member Author

One other comment... If I remember correctly, the last decision made regarding the disabled but ON switch when admin is selected for the Hide Permission switch was... not to show the switch, right?

I'm not sure, in design it's still present. @xAlisher, could you please take a look?

@noeliaSD
Copy link
Contributor

Thank you @noeliaSD for your comments!

  • The Edit flow is not correct. I.e: When you select Become member, then create the new permission, then go to Edit. The In selection should be the community one. Now it appears the option of selecting channels:

Could we address it in this dedicated task: #8855 ? I think it will be easier to do it completely there.

For me it is ok to address it separately. As far as we have it tracked in any task, it is ok.

Other comments not related to this pr but it would be nice if you also fix them as part of it:

  1. Read is View only in the design (PermissionsDropdown.qml)

Done

  1. Is allowwed to icons are black instead of blue when they are selected:
    Screenshot 2023-01-23 at 12 44 50

Yeah, I noticed that and tried to fix, but it requires a closer look imo. From what I saw it may need some more complex changes to have StatusItemSelector working properly with various sources of image icons. So I would opt for addressing it separately.

Ok. Could you add a new task to track it?

@micieslak
Copy link
Member Author

Thank you @noeliaSD for your comments!

  • The Edit flow is not correct. I.e: When you select Become member, then create the new permission, then go to Edit. The In selection should be the community one. Now it appears the option of selecting channels:

Could we address it in this dedicated task: #8855 ? I think it will be easier to do it completely there.

For me it is ok to address it separately. As far as we have it tracked in any task, it is ok.

Other comments not related to this pr but it would be nice if you also fix them as part of it:

  1. Read is View only in the design (PermissionsDropdown.qml)

Done

  1. Is allowwed to icons are black instead of blue when they are selected:
    Screenshot 2023-01-23 at 12 44 50

Yeah, I noticed that and tried to fix, but it requires a closer look imo. From what I saw it may need some more complex changes to have StatusItemSelector working properly with various sources of image icons. So I would opt for addressing it separately.

Ok. Could you add a new task to track it?

Sure thing: #9280

@glitchminer glitchminer self-requested a review January 25, 2023 14:14
Copy link
Contributor

@glitchminer glitchminer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA

Tested, approved.

Hi @micieslak, a few issues were noted in points 26-29 but from previous comments it looks like they are already known and will be addressed in other PRs. Let me know what you think. Thanks!


“Is allowed to” field

Admin granting permission

  1. OK: No option to grant admin rights
  2. OK: Option to grant Become member
  3. OK: Option to grant Moderate
  4. OK: Option to grant View and post
  5. OK: Option to grant View only

Owner granting permission

  1. OK: Option to grant admin rights
  2. OK: Option to grant Become member
  3. OK: Option to grant Moderate
  4. OK: Option to grant View and post
  5. OK: Option to grant View only

Descriptions
11. OK: Permission description for Become an admin
12. OK: Permission description for Become member
13. OK: Permission description for Moderate
14. OK: Permission description for View and post
15. OK: Permission description for View only


“In” field

  1. OK: When an “Become admin” admin is selected in “Is allowed to”, the value in “In” is fixed to the community name. No other option is available.
  2. OK: When “Become member” is selected in “Is allowed to”, the value in “In” is fixed to the community name. No other option is available.
  3. OK: When “Moderate” is selected in “Is allowed to”, the value in “In” can be 1 or more channels
  4. OK: When “View and post” is selected in “Is allowed to”, the value in “In” can be 1 or more channels
  5. OK: When View only is selected in “Is allowed to”, the value in “In” can be 1 or more channels

Default private permission option

  1. OK: Become admin: Enabled, not editable
  2. OK: Become member: Disabled, editable
  3. OK: Moderate: Enabled, editable
  4. OK: View and post: Disabled, editable
  5. OK: View only: Disabled, editable

Edit

  1. FAIL: Editing Become admin permission shows channels instead of community in “In” field
  2. FAIL: Editing Become member permission shows channels instead of community in “In” field
    image

Permissions list:

  1. FAIL: Become admin permission details show channels instead of community name
  2. FAIL: Become member permission details show channels instead of community name
    Expected
    image

Actual
image

@micieslak
Copy link
Member Author

Hi @glitchminer, as discussed earlier with Noelia, there is a separate task (#8855) to cover edit flow (points 26-29). Thanks for extensive testing!

@micieslak micieslak merged commit 63a0f51 into master Jan 26, 2023
@micieslak micieslak deleted the chore/issue-9050 branch January 26, 2023 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Community Permissions] Update Is allowed and In flows / logic according to the new design
7 participants