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.

Sign up for GitHub

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

Jump to bottom

[full-ci] Admin settings add group members panel #8826

Merged
merged 16 commits into from
Apr 19, 2023
Merged

[full-ci] Admin settings add group members panel #8826

merged 16 commits into from
Apr 19, 2023

Conversation

lookacat
Copy link
Contributor

@lookacat lookacat commented Apr 17, 2023 •
edited
Loading

Description

We've added the members panel to the groups page in admin settings.
For more details: #8596

Related Issue

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@lookacat lookacat marked this pull request as ready for review April 17, 2023 14:05
JammingBen
JammingBen reviewed Apr 17, 2023
View reviewed changes
packages/web-app-admin-settings/src/components/Spaces/SideBar/MembersPanel.vue Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm I'm honestly not the biggest fan of combining space- and group-members in one and the same panel. Especially when we extend the functionality in the future to search/filter by specific attributes or make it possible to add/remove members.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I was about to write the same. We did this already with the Quota Modal which looks super cluttery.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so some code duplication is better in this case, right? would be fine with me too

Copy link
Contributor

@JammingBen JammingBen Apr 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It's always a fine line between making components generic & reusable and duplicating code. Here I'd say a separate component with a bit of duplication is better. I suspect at least the group member panel will be expanded by some edit functionality in the future. That's the point where it definitely will get messy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

packages/web-app-admin-settings/src/views/Groups.vue Outdated Show resolved Hide resolved
@lookacat lookacat requested a review from JammingBen April 18, 2023 12:01
@lookacat lookacat requested a review from JammingBen April 18, 2023 12:57
@JammingBen
Copy link
Contributor

Code looks good to me! But searching gives me weird results 🤔 Does it happen for you as well @lookacat ?

image

@lookacat
Copy link
Contributor Author

no actually not :/ what did you do?

@lookacat
Copy link
Contributor Author

should now work fine

AlexAndBear
AlexAndBear approved these changes Apr 19, 2023
View reviewed changes
Copy link
Contributor

@AlexAndBear AlexAndBear left a comment

Choose a reason for hiding this comment

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

👍

kulmann
kulmann reviewed Apr 19, 2023
View reviewed changes
packages/web-app-admin-settings/tests/unit/components/Groups/SideBar/MembersPanel.spec.ts
@@ -0,0 +1,47 @@
import MembersPanel from '../../../../../src/components/Groups/SideBar/MembersPanel.vue'
Copy link
Member

@kulmann kulmann Apr 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

the import looks wrong (or at least weird 😆 )

kulmann
kulmann approved these changes Apr 19, 2023
View reviewed changes
Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

nice 👍

JammingBen
JammingBen suggested changes Apr 19, 2023
View reviewed changes
Copy link
Contributor

@JammingBen JammingBen left a comment

Choose a reason for hiding this comment

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

As discussed in chat: Highlighting the filter term does not work yet :(

@kulmann
Copy link
Member

kulmann commented Apr 19, 2023

As discussed in chat: Highlighting the filter term does not work yet :(

Oh true... and now found that having marie in a group e.g. filter term Curie doesn't find a result o_O When I search for Marie the highlighting works (but swallows the blank), searching for Mari nothing is highlighted.

JammingBen
JammingBen approved these changes Apr 19, 2023
View reviewed changes
Copy link
Contributor

@JammingBen JammingBen left a comment

Choose a reason for hiding this comment

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

Works now 👍

On a side note: the Fuse search in general is SUPER buggy though. I constantly get no results where there should definitely be some. Not only here, but basically everywhere we use it.

@ownclouders
Copy link
Contributor

Results for acceptance oC10 https://drone.owncloud.com/owncloud/web/34908/23/1
💥 The oC10SharingAccept tests pipeline failed. The build has been cancelled.

@ownclouders
Copy link
Contributor

Results for acceptance oC10 https://drone.owncloud.com/owncloud/web/34908/71/1
💥 The oC10IntegrationApp2 tests pipeline failed. The build has been cancelled.

@ownclouders
Copy link
Contributor

Results for acceptance oC10 https://drone.owncloud.com/owncloud/web/34908/26/1
💥 The oC10SharingIntGroups tests pipeline failed. The build has been cancelled.

@ownclouders
Copy link
Contributor

Results for acceptance oC10 https://drone.owncloud.com/owncloud/web/34908/25/1
💥 The oC10Sharing2 tests pipeline failed. The build has been cancelled.

@lookacat lookacat changed the title Admin settings add group members panel [full-ci] Admin settings add group members panel Apr 19, 2023
@lookacat lookacat force-pushed the admin-settings-add-group-members-panel branch from b878c2c to 9ffddfe Compare April 19, 2023 09:37
@sonarcloud
Copy link

sonarcloud bot commented Apr 19, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

86.9% 86.9% Coverage
0.0% 0.0% Duplication

@lookacat lookacat merged commit c966194 into master Apr 19, 2023
@delete-merged-branch delete-merged-branch bot deleted the admin-settings-add-group-members-panel branch April 19, 2023 10:11
@micbar micbar mentioned this pull request May 3, 2023
Closed
89 tasks
@ScharfViktor ScharfViktor mentioned this pull request May 4, 2023
Closed
86 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JammingBen JammingBen JammingBen approved these changes

@AlexAndBear AlexAndBear AlexAndBear approved these changes

@kulmann kulmann kulmann approved these changes

@dschmidt dschmidt Awaiting requested review from dschmidt

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[admin-settings] Add "Members" to group details
5 participants
@lookacat @JammingBen @kulmann @ownclouders @AlexAndBear