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
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
6f48e0a
fix: make readme file links relative so they are clickable again
fhennig Sep 20, 2024
6fd03ec
feat: add jq test to script; use 'env' instead of 'bash' directly
fhennig Sep 20, 2024
18de929
add some documentation to the Kubernetes README
fhennig Sep 20, 2024
97c3db0
WIP - adding the backend stuff
fhennig Sep 20, 2024
545ea5b
move port info higher up in the README
fhennig Sep 20, 2024
12d5ae4
Add TODO for test
fhennig Sep 20, 2024
7656622
Add more TODOs in relevant files so I don't forget where to change stuff
fhennig Sep 20, 2024
7362eac
Add code to update the group
fhennig Sep 27, 2024
0dc21ca
linter fixes
fhennig Sep 27, 2024
6872d7f
Add docker check to gradle to prevent cryptic test failures due to do…
fhennig Sep 27, 2024
8c44d79
Add test
fhennig Sep 27, 2024
e680f92
Formatting
fhennig Sep 27, 2024
3bdb175
Add Edit group button (WIP)
fhennig Sep 27, 2024
86f2a5b
Add edit page stub and routing
fhennig Sep 30, 2024
8530a68
1st working version
fhennig Sep 30, 2024
510feb6
a bit of cleanup
fhennig Sep 30, 2024
a4df845
fixed a todo
fhennig Sep 30, 2024
9e8136c
cleanup field mapping stuff
fhennig Sep 30, 2024
2b626a4
fix broken test
fhennig Sep 30, 2024
a4a0bca
address review
fhennig Sep 30, 2024
d7554f6
Remove todo
fhennig Oct 1, 2024
a70c205
turn button into anchor tag
fhennig Oct 2, 2024
1ced011
document where group info should be propagated
corneliusroemer Oct 3, 2024
155eb23
Document limitation in README
corneliusroemer Oct 3, 2024
fa2842a
Clarify that group name is never used in ingest pipeline other than f…
corneliusroemer Oct 3, 2024
8d65082
Document group info updatability
corneliusroemer Oct 3, 2024
f34b42f
Add stub for group info update -> released data integration test
corneliusroemer Oct 3, 2024
5d6d5e7
Add todo for website e2e test
corneliusroemer Oct 3, 2024
c1fce8d
Automated backend code formatting
Oct 3, 2024
18c8524
Update backend/src/test/kotlin/org/loculus/backend/controller/groupma…
fhennig Oct 3, 2024
cf5d7df
Add test for 'alternative user can't edit group
fhennig Oct 3, 2024
00e5f3a
Add test for 'superuser can edit group'
fhennig Oct 3, 2024
d409448
Refactor testing
fhennig Oct 3, 2024
b77c665
Refactor testing
fhennig Oct 3, 2024
2d15399
Update loculus docs
fhennig Oct 3, 2024
8c65e21
test(backend): Test that updated group info ends up in released data …
fhennig Oct 4, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,9 @@ Additional documentation for development is available in each folder's README. T

If you would like to develop with a full local loculus instance for development you need to:

1. Deploy a local kubernetes instance: [kubernetes](/kubernetes/README.md)
2. Deploy the backend: [backend](/backend/README.md)
3. Deploy the frontend/website: [website](/website/README.md)
1. Deploy a local kubernetes instance: [kubernetes](./kubernetes/README.md)
2. Deploy the backend: [backend](./backend/README.md)
3. Deploy the frontend/website: [website](./website/README.md)

Note that if you are developing the backend or frontend/website in isolation a full local loculus instance is not required. See the individual READMEs for more information.

Expand Down Expand Up @@ -79,6 +79,7 @@ For testing we added multiple users to the realm. The users are:
- Each user can be a member of multiple submitting groups.
- Users can create new submitting groups, becoming the initial member automatically.
- Group members have the authority to add or remove other members.
- Group members have the authority to edit all group metadata (except for group id)
- If the last user leaves a submitting group, the group becomes 'dangling'—it exists but is no longer accessible, and a new group with the same name cannot be created.
- Admin users can manually delete a submitting group directly on the DB but must transfer ownership of sequence entries to another submitting group before doing so to fulfill the foreign key constraint.

Expand Down
4 changes: 2 additions & 2 deletions backend/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ All commands mentioned in this section are run from the `backend` directory unle
./start_dev.sh
```

The service listens, by default, to **port 8079**: <http://localhost:8079/swagger-ui/index.html>.

3. Clean up the database when done:

```sh
Expand Down Expand Up @@ -67,8 +69,6 @@ You need to set:

We use Flyway, so that the service can provision an empty/existing DB without any manual steps in between. On startup scripts in `src/main/resources/db/migration` are executed in order, i.e. `V1__*.sql` before `V2__*.sql` if they didn't run before, so that the DB is always up-to-date. (For more info on the naming convention, see [this](https://www.red-gate.com/blog/database-devops/flyway-naming-patterns-matter) blog post.)

The service listens, by default, to **port 8079**: <http://localhost:8079/swagger-ui/index.html>.

Note: When using a postgresSQL development platform (e.g. pgAdmin) the hostname is 127.0.0.1 and not localhost - this is defined in the `deploy.py` file.

Note that we also use flyway in the ena-submission pod to create an additional schema in the database, ena-submission. This schema is not added here.
Expand Down
18 changes: 18 additions & 0 deletions backend/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,25 @@ dependencies {
}
}

// Check if the docker engine is running and reachable
task checkDocker {
doLast {
def process = "docker info".execute()
def output = new StringWriter()
def error = new StringWriter()
process.consumeProcessOutput(output, error)
process.waitFor()

if (process.exitValue() != 0) {
throw new GradleException("Docker is not running: ${error.toString()}")
}
println "Docker is running."
}
}

tasks.named('test') {
// Docker is required to start the testing database
dependsOn checkDocker
useJUnitPlatform()
testLogging {
events TestLogEvent.FAILED
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,24 @@ class GroupManagementController(private val groupManagementDatabaseService: Grou
@PostMapping("/groups", produces = [MediaType.APPLICATION_JSON_VALUE])
fun createNewGroup(
@HiddenParam authenticatedUser: AuthenticatedUser,
@Parameter(description = "Information about the newly created group")
@Parameter(description = "Information about the newly created group.")
@RequestBody
group: NewGroup,
): Group = groupManagementDatabaseService.createNewGroup(group, authenticatedUser)

@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)

@Operation(description = "Get details of a group.")
@ResponseStatus(HttpStatus.OK)
@GetMapping("/groups/{groupId}", produces = [MediaType.APPLICATION_JSON_VALUE])
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,35 +32,14 @@ class GroupManagementDatabaseService(
val users = UserGroupEntity.find { UserGroupsTable.groupIdColumn eq groupId }

return GroupDetails(
group = Group(
groupId = groupEntity.id.value,
groupName = groupEntity.groupName,
institution = groupEntity.institution,
address = Address(
line1 = groupEntity.addressLine1,
line2 = groupEntity.addressLine2,
postalCode = groupEntity.addressPostalCode,
city = groupEntity.addressCity,
state = groupEntity.addressState,
country = groupEntity.addressCountry,
),
contactEmail = groupEntity.contactEmail,
),
group = groupEntity.toGroup(),
users = users.map { User(it.userName) },
)
}

fun createNewGroup(group: NewGroup, authenticatedUser: AuthenticatedUser): Group {
val groupEntity = GroupEntity.new {
groupName = group.groupName
institution = group.institution
addressLine1 = group.address.line1
addressLine2 = group.address.line2
addressPostalCode = group.address.postalCode
addressState = group.address.state
addressCity = group.address.city
addressCountry = group.address.country
contactEmail = group.contactEmail
this.updateWith(group)
}

val groupId = groupEntity.id.value
Expand All @@ -72,20 +51,21 @@ class GroupManagementDatabaseService(

auditLogger.log(authenticatedUser.username, "Created group: ${group.groupName}")

return Group(
groupId = groupEntity.id.value,
groupName = groupEntity.groupName,
institution = groupEntity.institution,
address = Address(
line1 = groupEntity.addressLine1,
line2 = groupEntity.addressLine2,
postalCode = groupEntity.addressPostalCode,
city = groupEntity.addressCity,
state = groupEntity.addressState,
country = groupEntity.addressCountry,
),
contactEmail = groupEntity.contactEmail,
)
return groupEntity.toGroup()
}

fun updateGroup(groupId: Int, group: NewGroup, authenticatedUser: AuthenticatedUser): Group {
groupManagementPreconditionValidator.validateThatUserExists(authenticatedUser.username)

groupManagementPreconditionValidator.validateUserIsAllowedToModifyGroup(groupId, authenticatedUser)

val groupEntity = GroupEntity.findById(groupId) ?: throw NotFoundException("Group $groupId does not exist.")

groupEntity.updateWith(group)

auditLogger.log(authenticatedUser.username, "Updated group: ${group.groupName}")

return groupEntity.toGroup()
}

fun getGroupsOfUser(authenticatedUser: AuthenticatedUser): List<Group> {
Expand Down Expand Up @@ -172,4 +152,31 @@ class GroupManagementDatabaseService(
contactEmail = it.contactEmail,
)
}

private fun GroupEntity.updateWith(group: NewGroup) {
fhennig marked this conversation as resolved.
Show resolved Hide resolved
fhennig marked this conversation as resolved.
Show resolved Hide resolved
groupName = group.groupName
institution = group.institution
addressLine1 = group.address.line1
addressLine2 = group.address.line2
addressPostalCode = group.address.postalCode
addressState = group.address.state
addressCity = group.address.city
addressCountry = group.address.country
contactEmail = group.contactEmail
}

private fun GroupEntity.toGroup(): Group = Group(
groupId = this.id.value,
groupName = this.groupName,
institution = this.institution,
address = Address(
line1 = this.addressLine1,
line2 = this.addressLine2,
postalCode = this.addressPostalCode,
city = this.addressCity,
state = this.addressState,
country = this.addressCountry,
),
contactEmail = this.contactEmail,
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,14 @@ class GroupManagementControllerClient(private val mockMvc: MockMvc, private val
.withAuth(jwt),
)

fun updateGroup(groupId: Int, group: NewGroup = NEW_GROUP, jwt: String? = jwtForDefaultUser): ResultActions =
mockMvc.perform(
put("/groups/$groupId")
.contentType(MediaType.APPLICATION_JSON_VALUE)
.content(objectMapper.writeValueAsString(group))
.withAuth(jwt),
)

fun getGroupsOfUser(jwt: String? = jwtForDefaultUser): ResultActions = mockMvc.perform(
get("/user/groups").withAuth(jwt),
)
Expand Down
fhennig marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import org.junit.jupiter.api.Test
import org.junit.jupiter.params.ParameterizedTest
import org.junit.jupiter.params.provider.MethodSource
import org.keycloak.representations.idm.UserRepresentation
import org.loculus.backend.api.Address
import org.loculus.backend.api.NewGroup
import org.loculus.backend.controller.ALTERNATIVE_DEFAULT_GROUP
import org.loculus.backend.controller.ALTERNATIVE_DEFAULT_GROUP_NAME
import org.loculus.backend.controller.ALTERNATIVE_DEFAULT_USER_NAME
Expand Down Expand Up @@ -104,6 +106,50 @@ class GroupManagementControllerTest(@Autowired private val client: GroupManageme
.andExpect(jsonPath("\$.[0].contactEmail").value(NEW_GROUP.contactEmail))
}

@Test
fun `GIVEN a group is created WHEN I edit the group THEN the group information is updated`() {
fhennig marked this conversation as resolved.
Show resolved Hide resolved
val groupId = client.createNewGroup(group = DEFAULT_GROUP, jwt = jwtForDefaultUser)
.andExpect(status().isOk)
.andGetGroupId()
val newInfo = NewGroup(
groupName = "Updated group name",
institution = "Updated institution",
address = Address(
line1 = "Updated address line 1",
line2 = "Updated address line 2",
postalCode = "Updated post code",
city = "Updated city",
state = "Updated state",
country = "Updated country",
),
contactEmail = "Updated email",
)
client.updateGroup(groupId = groupId, group = newInfo, jwt = jwtForDefaultUser)
.andExpect(status().isOk)
.andExpect(content().contentType(MediaType.APPLICATION_JSON_VALUE))
.andExpect(jsonPath("\$.groupName").value(newInfo.groupName))
.andExpect(jsonPath("\$.institution").value(newInfo.institution))
.andExpect(jsonPath("\$.address.line1").value(newInfo.address.line1))
.andExpect(jsonPath("\$.address.line2").value(newInfo.address.line2))
.andExpect(jsonPath("\$.address.city").value(newInfo.address.city))
.andExpect(jsonPath("\$.address.state").value(newInfo.address.state))
.andExpect(jsonPath("\$.address.postalCode").value(newInfo.address.postalCode))
.andExpect(jsonPath("\$.address.country").value(newInfo.address.country))
.andExpect(jsonPath("\$.contactEmail").value(newInfo.contactEmail))
client.getDetailsOfGroup(groupId = groupId, jwt = jwtForDefaultUser)
.andExpect(status().isOk)
.andExpect(content().contentType(MediaType.APPLICATION_JSON_VALUE))
.andExpect(jsonPath("\$.group.groupName").value(newInfo.groupName))
.andExpect(jsonPath("\$.group.institution").value(newInfo.institution))
.andExpect(jsonPath("\$.group.address.line1").value(newInfo.address.line1))
.andExpect(jsonPath("\$.group.address.line2").value(newInfo.address.line2))
.andExpect(jsonPath("\$.group.address.city").value(newInfo.address.city))
.andExpect(jsonPath("\$.group.address.state").value(newInfo.address.state))
.andExpect(jsonPath("\$.group.address.postalCode").value(newInfo.address.postalCode))
.andExpect(jsonPath("\$.group.address.country").value(newInfo.address.country))
.andExpect(jsonPath("\$.group.contactEmail").value(newInfo.contactEmail))
}

fhennig marked this conversation as resolved.
Show resolved Hide resolved
@Test
fun `WHEN superuser queries groups of user THEN returns all groups`() {
client.createNewGroup(group = DEFAULT_GROUP, jwt = jwtForDefaultUser)
Expand Down
4 changes: 4 additions & 0 deletions ena-submission/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ In a loop:
- else: set status to HAS_ERRORS and fill in errors
- Get sequences in `project_table` in state HAS_ERRORS for over 15min and sequences in status SUBMITTING for over 15min: send slack notification

##### Known limitations

Group info can be updated in loculus after the project has been created in ENA. This is not currently handled by the pipeline. Issue: <https://github.com/loculus-project/loculus/issues/2939>

#### create_sample

Maps loculus metadata to ena metadata using template: https://www.ebi.ac.uk/ena/browser/view/ERC000033
Expand Down
1 change: 0 additions & 1 deletion ena-submission/scripts/call_loculus.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ class Config:
keycloak_client_id: str
username: str
password: str
group_name: str
ena_specific_metadata: list[str]


Expand Down
1 change: 1 addition & 0 deletions ena-submission/scripts/create_project.py
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ def submission_table_update(db_config: SimpleConnectionPool):
raise RuntimeError(error_msg)


# TODO Allow propagating updated group info https://github.com/loculus-project/loculus/issues/2939
def project_table_create(
db_config: SimpleConnectionPool, config: Config, retry_number: int = 3, test: bool = False
):
Expand Down
2 changes: 1 addition & 1 deletion generate_local_test_config.sh
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#!/bin/bash
#!/usr/bin/env bash

set -e

Expand Down
8 changes: 7 additions & 1 deletion get_testuser_token.sh
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
#!/usr/bin/bash
#!/usr/bin/env bash
set -eu

# Check if jq is installed
if ! command -v jq &> /dev/null; then
echo "Error: jq is not installed. Please install it from https://jqlang.github.io/jq/"
exit 1
fi

KEYCLOAK_TOKEN_URL="http://localhost:8083/realms/loculus/protocol/openid-connect/token"
KEYCLOAK_CLIENT_ID="backend-client"

Expand Down
2 changes: 1 addition & 1 deletion ingest/config/defaults.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ column_mapping:
Virus Name: ncbiVirusName
Virus Pangolin Classification: ncbiVirusPangolin
Virus Taxonomic ID: ncbiVirusTaxId
group_name: insdc_ingest_group
group_name: insdc_ingest_group # Used only to set the group name, never read
username: insdc_ingest_user
password: insdc_ingest_user
keycloak_client_id: backend-client
Expand Down
20 changes: 13 additions & 7 deletions ingest/scripts/call_loculus.py
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ def make_request( # noqa: PLR0913, PLR0917
return response


def create_group(config: Config) -> str:
def create_group_and_return_group_id(config: Config) -> str:
create_group_url = f"{backend_url(config)}/groups"
group_name = config.group_name

Expand Down Expand Up @@ -151,7 +151,7 @@ def create_group(config: Config) -> str:
return group_id


def get_or_create_group(config: Config, allow_creation: bool = False) -> str:
def get_or_create_group_and_return_group_id(config: Config, allow_creation: bool = False) -> str:
"""Returns group id"""
get_user_groups_url = f"{backend_url(config)}/user/groups"

Expand All @@ -168,7 +168,7 @@ def get_or_create_group(config: Config, allow_creation: bool = False) -> str:
raise ValueError(msg)

logger.info("User is not in any group. Creating a new group")
return create_group(config)
return create_group_and_return_group_id(config)


def submit_or_revise(
Expand Down Expand Up @@ -332,8 +332,10 @@ def get_submitted(config: Config):
if len(entries) == expected_record_count:
f"Got {len(entries)} records as expected"
break
logger.error(f"Got incomplete original metadata stream: expected {len(entries)}"
f"records but got {expected_record_count}. Retrying after 60 seconds.")
logger.error(
f"Got incomplete original metadata stream: expected {len(entries)}"
f"records but got {expected_record_count}. Retrying after 60 seconds."
)
sleep(60)

# Initialize the dictionary to store results
Expand Down Expand Up @@ -467,7 +469,9 @@ def record_factory(*args, **kwargs):
if mode in {"submit", "revise"}:
logging.info(f"Starting {mode}")
try:
group_id = get_or_create_group(config, allow_creation=mode == "submit")
group_id = get_or_create_group_and_return_group_id(
config, allow_creation=mode == "submit"
)
except ValueError as e:
logger.error(f"Aborting {mode} due to error: {e}")
return
Expand All @@ -487,7 +491,9 @@ def record_factory(*args, **kwargs):

if mode == "regroup-and-revoke":
try:
group_id = get_or_create_group(config, allow_creation=mode == "submit")
group_id = get_or_create_group_and_return_group_id(
config, allow_creation=mode == "submit"
)
except ValueError as e:
logger.error(f"Aborting {mode} due to error: {e}")
return
Expand Down
Loading
Loading