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 all 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
11 changes: 7 additions & 4 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 All @@ -85,7 +85,10 @@ When running the backend behind a proxy, the proxy needs to set X-Forwarded head

### Run tests and lints

The tests use Testcontainers to start a PostgreSQL database. This requires Docker or a Docker-API compatible container runtime to be installed, and the user executing the test needs the necessary permissions to use it. See [the documentation of the Testcontainers](https://java.testcontainers.org/supported_docker_environment/) for details.
The tests use [Testcontainers](https://testcontainers.com/) to start a PostgreSQL database.
This requires Docker or a Docker-API compatible container runtime to be installed and running,
and the user executing the test needs the necessary permissions to use it.
See [the documentation of the Testcontainers](https://java.testcontainers.org/supported_docker_environment/) for details.

```bash
./gradlew test
Expand All @@ -97,7 +100,7 @@ The tests use Testcontainers to start a PostgreSQL database. This requires Docke
./gradlew ktlintCheck
```

## Format
### Format

```bash
./gradlew ktlintFormat
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 with https://java.testcontainers.org/
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 @@ -54,6 +54,7 @@ private const val SPRING_DATASOURCE_PASSWORD = "spring.datasource.password"

const val ACCESSION_SEQUENCE_NAME = "accession_sequence"
const val DEFAULT_GROUP_NAME = "testGroup"
const val DEFAULT_GROUP_NAME_CHANGED = "testGroup name updated"
val DEFAULT_GROUP = NewGroup(
groupName = DEFAULT_GROUP_NAME,
institution = "testInstitution",
Expand All @@ -67,6 +68,19 @@ val DEFAULT_GROUP = NewGroup(
),
contactEmail = "testEmail",
)
val DEFAULT_GROUP_CHANGED = NewGroup(
groupName = DEFAULT_GROUP_NAME_CHANGED,
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",
)

const val DEFAULT_USER_NAME = "testuser"
const val SUPER_USER_NAME = "test_superuser"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import java.util.Date
val keyPair: KeyPair = Jwts.SIG.RS256.keyPair().build()

val jwtForDefaultUser = generateJwtFor(DEFAULT_USER_NAME)
val jwtForAlternativeUser = generateJwtFor(ALTERNATIVE_DEFAULT_USER_NAME)
val jwtForProcessingPipeline = generateJwtFor("preprocessing_pipeline", listOf(PREPROCESSING_PIPELINE))
val jwtForExternalMetadataUpdatePipeline =
generateJwtFor("external_metadata_updater", listOf(EXTERNAL_METADATA_UPDATER))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ import org.springframework.test.web.servlet.request.MockMvcRequestBuilders.delet
import org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get
import org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post
import org.springframework.test.web.servlet.request.MockMvcRequestBuilders.put
import org.springframework.test.web.servlet.result.MockMvcResultMatchers.content
import org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath
import org.springframework.test.web.servlet.result.MockMvcResultMatchers.status

const val NEW_GROUP_NAME = "newGroup"
val NEW_GROUP = NewGroup(
Expand Down Expand Up @@ -49,6 +52,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 All @@ -74,3 +85,15 @@ fun ResultActions.andGetGroupId(): Int = andReturn()
.response
.contentAsString
.let { JsonPath.read(it, "\$.groupId") }!!

fun ResultActions.verifyGroupInfo(groupPath: String, expectedGroup: NewGroup) = andExpect(status().isOk())
.andExpect(content().contentType(MediaType.APPLICATION_JSON_VALUE))
.andExpect(jsonPath("$groupPath.groupName").value(expectedGroup.groupName))
.andExpect(jsonPath("$groupPath.institution").value(expectedGroup.institution))
.andExpect(jsonPath("$groupPath.address.line1").value(expectedGroup.address.line1))
.andExpect(jsonPath("$groupPath.address.line2").value(expectedGroup.address.line2))
.andExpect(jsonPath("$groupPath.address.city").value(expectedGroup.address.city))
.andExpect(jsonPath("$groupPath.address.state").value(expectedGroup.address.state))
.andExpect(jsonPath("$groupPath.address.postalCode").value(expectedGroup.address.postalCode))
.andExpect(jsonPath("$groupPath.address.country").value(expectedGroup.address.country))
.andExpect(jsonPath("$groupPath.contactEmail").value(expectedGroup.contactEmail))
fhennig marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,13 @@ 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
import org.loculus.backend.controller.DEFAULT_GROUP
import org.loculus.backend.controller.DEFAULT_GROUP_CHANGED
import org.loculus.backend.controller.DEFAULT_GROUP_NAME
import org.loculus.backend.controller.DEFAULT_USER_NAME
import org.loculus.backend.controller.EndpointTest
import org.loculus.backend.controller.expectUnauthorizedResponse
import org.loculus.backend.controller.generateJwtFor
import org.loculus.backend.controller.jwtForAlternativeUser
import org.loculus.backend.controller.jwtForDefaultUser
import org.loculus.backend.controller.jwtForSuperUser
import org.loculus.backend.service.KeycloakAdapter
Expand Down Expand Up @@ -104,6 +106,56 @@ class GroupManagementControllerTest(@Autowired private val client: GroupManageme
.andExpect(jsonPath("\$.[0].contactEmail").value(NEW_GROUP.contactEmail))
}

@Test
fun `GIVEN I'm a member of a group WHEN I edit the group THEN the group information is updated`() {
val groupId = client.createNewGroup(group = DEFAULT_GROUP, jwt = jwtForDefaultUser)
.andExpect(status().isOk)
.andGetGroupId()

client.updateGroup(
groupId = groupId,
group = DEFAULT_GROUP_CHANGED,
jwt = jwtForDefaultUser,
).verifyGroupInfo("\$", DEFAULT_GROUP_CHANGED)

client.getDetailsOfGroup(groupId = groupId, jwt = jwtForDefaultUser)
.verifyGroupInfo("\$.group", DEFAULT_GROUP_CHANGED)
}

@Test
fun `GIVEN I'm a superuser WHEN I edit the group THEN the group information is updated`() {
val groupId = client.createNewGroup(group = DEFAULT_GROUP, jwt = jwtForDefaultUser)
.andExpect(status().isOk)
.andGetGroupId()

client.updateGroup(
groupId = groupId,
group = DEFAULT_GROUP_CHANGED,
jwt = jwtForSuperUser,
)
.verifyGroupInfo("\$", DEFAULT_GROUP_CHANGED)

client.getDetailsOfGroup(groupId = groupId, jwt = jwtForSuperUser)
.verifyGroupInfo("\$.group", DEFAULT_GROUP_CHANGED)
}

@Test
fun `GIVEN I'm not a member of a group WHEN I edit the group THEN I am not authorized`() {
val groupId = client.createNewGroup(group = DEFAULT_GROUP, jwt = jwtForDefaultUser)
.andExpect(status().isOk)
.andGetGroupId()

val updateGroupResult = client.updateGroup(
groupId = groupId,
group = DEFAULT_GROUP_CHANGED,
jwt = jwtForAlternativeUser,
)
updateGroupResult.andExpect(status().isForbidden)

client.getDetailsOfGroup(groupId = groupId, jwt = jwtForDefaultUser)
.verifyGroupInfo("\$.group", DEFAULT_GROUP)
}

@Test
fun `WHEN superuser queries groups of user THEN returns all groups`() {
client.createNewGroup(group = DEFAULT_GROUP, jwt = jwtForDefaultUser)
Expand Down
Loading
Loading