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

feat: Edit group info #2854

Open
wants to merge 36 commits into
base: main
Choose a base branch
from
Open

feat: Edit group info #2854

wants to merge 36 commits into from

Conversation

fhennig
Copy link

@fhennig fhennig commented Sep 20, 2024

resolves #2258

preview URL: https://feat-2258-edit-group-info.loculus.org

Summary

This PR adds a new backend API endpoint PUT /group/<groupId> as well as a group edit button to the group details view in the UI. Only group members can edit the group. All group info can be edited (except the group ID).

Screenshot

Screen Recording 2024-10-01 at 17 47 52

PR Checklist

  • All necessary documentation has been adapted.
    • Warn users that if they change group name they will change ENA center name
  • The implemented feature is covered by an appropriate test:
    • Manual tests:
      • 1. make an account, 2. create group, 3. submit sequences, 4. change everything in group, 5. see if we can still revise, etc.
      • In staging, change everything for grubaugh lab and see if it causes issues, e.g. also adding new sequences and seeing if ENA is happy: @anna-parker and @corneliusroemer should do this
    • Automatic tests:
      • Add end-to-end tests for the manual scenario
  • Thought about potential security implications:
    • Ensure id can't be changed
    • We never use group name anywhere for anything security relevant
    • Ensure only group members can change group info

Copy link
Contributor

@corneliusroemer corneliusroemer left a comment

Choose a reason for hiding this comment

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

Looks great. Good start!

Copy link
Member

@chaoran-chen chaoran-chen left a comment

Choose a reason for hiding this comment

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

Nice code outlines and great to see the small docs and script improvements! If you create small PRs, I think that we can already merge them.

@fhennig
Copy link
Author

fhennig commented Sep 27, 2024

I'm done with the API part of the change, next is the UI.

I've also made a small change to the gradle build; next time I'll split it into a separate PR (just like the script changes).

@fhennig fhennig self-assigned this Sep 27, 2024
@corneliusroemer corneliusroemer added the preview Triggers a deployment to argocd label Sep 27, 2024
Copy link
Contributor

@corneliusroemer corneliusroemer left a comment

Choose a reason for hiding this comment

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

Not blocking but there seems to be quite some probably unnecessary repetition of the group fields - already from the legacy code, but now the duplication becomes more noticeable

We could make an issue to cleanup later, it's not necessarily pressing, just a bit of code smell (not your fault by the way, legacy)

website/src/components/Group/GroupForm.tsx Show resolved Hide resolved
website/src/components/Group/GroupForm.tsx Show resolved Hide resolved
@corneliusroemer
Copy link
Contributor

Cool! I tested and it works, you could record a quick Screencast to showcase :)

website/src/components/Group/GroupForm.tsx Show resolved Hide resolved
website/src/components/Group/GroupForm.tsx Outdated Show resolved Hide resolved
website/src/components/Group/Inputs.tsx Outdated Show resolved Hide resolved
website/src/components/Group/Inputs.tsx Outdated Show resolved Hide resolved
website/src/components/Group/GroupForm.tsx Show resolved Hide resolved
website/src/components/Group/GroupForm.tsx Outdated Show resolved Hide resolved
@fhennig
Copy link
Author

fhennig commented Sep 30, 2024

I've refactored the field mapping a bit, but I don't have an idea for how to completely get rid of a mapping function from form data to NewGroup -- my Typescript skills aren't good enough for that yet and ChatGPT didn't have a good idea either 😄

For the repetitive form inputs, I'd say it's fine for now. It's repetitive but simple to deal with. I am of course happy to change it if you have a concrete suggestion what to do.

@fhennig fhennig marked this pull request as ready for review September 30, 2024 15:26
Copy link
Contributor

@fengelniederhammer fengelniederhammer left a comment

Choose a reason for hiding this comment

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

I quickly looked over the code. Looks good from my side. But it would be good if someone has a closer look.

website/src/components/User/GroupPage.tsx Outdated Show resolved Hide resolved
@fhennig
Copy link
Author

fhennig commented Oct 2, 2024

@corneliusroemer I think this is a good point: "Warn users that if they change group name they will change ENA center name" -- How should we do this? Maybe we can even add a little note to the edit UI if the group name is edited (some text in yellow next to the edit field maybe?)

Regarding "group ID can't be changed" -- This is the definition of the endpoint:

    @Operation(description = "Edit a group. Only users part of the group can edit it. The updated group is returned.")
    @ResponseStatus(HttpStatus.OK)
    @PutMapping("/groups/{groupId}", produces = [MediaType.APPLICATION_JSON_VALUE])
    fun editGroup(
        @HiddenParam authenticatedUser: AuthenticatedUser,
        @Parameter(
            description = "The id of the group to edit.",
        ) @PathVariable groupId: Int,
        @Parameter(description = "Updated group properties.")
        @RequestBody
        group: NewGroup,
    ): Group = groupManagementDatabaseService.updateGroup(groupId, group, authenticatedUser)

The ID is part of the path and not part of the NewGroup, so I would say that's sufficient? By definition the ID can't be changed.

For "Ensure only group members can change group info" I will add another backend unit test.

@corneliusroemer
Copy link
Contributor

corneliusroemer commented Oct 3, 2024

This is where we use group info in cached ways:

  • ENA submission: group info used for project creation, currently not updated, see Propagate group edits to ENA projects #2939
  • Ingest: Only sets group name of ingest group, never uses it
  • Prepro: Never uses any group info apparently, even though group id is in data returned from prepro AFAICT

Backend tests:

Website tests:

Manual tests:

  • On staging: edit a bunch of groups and see what happens: needs PR on pathoplexus pointing at this PR commit hash, and also point loculus deployments at PP PR hash

Loculus docs updates:

  • Mention that groups can be edited somewhere appropriate -- 2d15399

Pathoplexus docs updates:

  • Mention where appropriate

@corneliusroemer corneliusroemer added the format_me Triggers github_actions to format website code on PR label Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
format_me Triggers github_actions to format website code on PR preview Triggers a deployment to argocd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow editing group contact information through website
4 participants