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

Add user membership management to group management #219

Merged
merged 29 commits into from
Jan 29, 2023
Merged

Conversation

kbroschke
Copy link
Collaborator

closes #106

@KiruChaff
Copy link
Collaborator

We've tried implementing the feature through a form which is supposed to call the add_user method in the group controller. Unfortunately this method is not called through.

Maybe validate the resource and check if the form is correct

@TobiPeterG
Copy link
Collaborator

Unfortunately, I don't have a second user account to test this feature; is there any easy way to add a new user to the database?

@DieKautz
Copy link
Collaborator

Whats still missing is

  • handling if the specified email belongs to no existing user and
  • shrinking the class length of the groups controller
    membership management maybe extracted to its own controller?

@kbroschke
Copy link
Collaborator Author

I implemented the idea of moving the user management logic into a separate controller. But there is still duplicated code left, because sharing code between controllers seems to be non-trivial. I already looked into using Concerns. Before using a separate controller, I also experimented with other solutions to our problem. But in the end, just pointing the routes to the new MembershipsController was the easiest one to implement.

@kbroschke kbroschke marked this pull request as ready for review January 22, 2023 16:24
@TobiPeterG
Copy link
Collaborator

When a user is not found, an alert should be shown instead of a notice. I added that.
Also there was an unused function added in the GroupsController, I removed it.
Might be useful for a refactor, but not this issue here.
Otherwise, it looks good!

Copy link
Collaborator

@TobiPeterG TobiPeterG left a comment

Choose a reason for hiding this comment

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

If someone could quickly look over my changes, we're good to merge!

@kbroschke
Copy link
Collaborator Author

kbroschke commented Jan 25, 2023

I can't explicitly approve my own pr, but everything looks good to me. At least with refactorings tracked in #346 and in #353.
Maybe at least one additional person should review all changes (@KiruChaff or @DieKautz?) because Tobias and I made so many recent changes.

@kbroschke kbroschke merged commit d2a96f6 into dev Jan 29, 2023
@kbroschke kbroschke deleted the ba/group-add-user branch January 29, 2023 19:14
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.

Add user membership management to group management
6 participants