fix(backend): deleting/approving all sequences only affects entries of that organism

[fengelniederhammer]

resolves #1594

preview URL:

Summary

Tests that approvedProcessedData and deleteSequenceEntryVersions only target the respective organism exist only when providing an accession filter:

loculus/backend/src/test/kotlin/org/loculus/backend/controller/submission/ApproveProcessedDataEndpointTest.kt

Lines 195 to 224 in 8749ddc

	@Test
	fun `WHEN I approve sequence entries of different organisms THEN request should be rejected`() {
	val defaultOrganismData = convenienceClient.prepareDataTo(AWAITING_APPROVAL, organism = DEFAULT_ORGANISM)
	val otherOrganismData = convenienceClient.prepareDataTo(AWAITING_APPROVAL, organism = OTHER_ORGANISM)
	client.approveProcessedSequenceEntries(
	scope = ALL,
	defaultOrganismData.getAccessionVersions() + otherOrganismData.getAccessionVersions(),
	organism = OTHER_ORGANISM,
	)
	.andExpect(status().isUnprocessableEntity)
	.andExpect(content().contentType(MediaType.APPLICATION_JSON))
	.andExpect(
	jsonPath("$.detail")
	.value(containsString("accession versions are not of organism otherOrganism")),
	)
	convenienceClient.getSequenceEntry(
	accession = defaultOrganismData.first().accession,
	version = 1,
	organism = DEFAULT_ORGANISM,
	)
	.assertStatusIs(AWAITING_APPROVAL)
	convenienceClient.getSequenceEntry(
	accession = otherOrganismData.first().accession,
	version = 1,
	organism = OTHER_ORGANISM,
	)
	.assertStatusIs(AWAITING_APPROVAL)
	}

loculus/backend/src/test/kotlin/org/loculus/backend/controller/submission/DeleteSequencesEndpointTest.kt

Lines 218 to 232 in 8749ddc

	@Test
	fun `WHEN deleting sequence entry of wrong organism THEN throws an unprocessableEntity error`() {
	val accessionVersion = convenienceClient.submitDefaultFiles(organism = DEFAULT_ORGANISM).submissionIdMappings[0]
	client.deleteSequenceEntries(
	scope = ALL,
	accessionVersionsFilter = listOf(accessionVersion.toAccessionVersion()),
	organism = OTHER_ORGANISM,
	)
	.andExpect(status().isUnprocessableEntity)
	.andExpect(content().contentType(MediaType.APPLICATION_JSON_VALUE))
	.andExpect(
	jsonPath("\$.detail", containsString("accession versions are not of organism $OTHER_ORGANISM:")),
	)
	}

I added tests and fixed the behavior when only the All scope is provided.

From several past discussion it was clear that the interface of the AccessionPreconditionValidator didn't really say what it was actually doing. So I refactored it in a way that should make it quite obvious what is validated (both for reading and for writing code):

        accessionPreconditionValidator.validate {
            thatAccessionsExist(accessions)
                .andThatSequenceEntriesAreInStates(listOf(APPROVED_FOR_RELEASE))
        }

Screenshot

PR Checklist

• [ ] All necessary documentation has been adapted.
• The implemented feature is covered by an appropriate test.

[chaoran-chen]

Do the tests really cover the case that I described in #1594? As far as I can see, the tests only cover the case where the user provides a list of accessions and the organism check only happens if a list of accessions is provided. However, if the user wants to approve or delete all sequences of a particular organism (without providing a list of accessions), we don't filter by that organism.

[fengelniederhammer]

Ah, good point. I'll check again.

[fengelniederhammer]

Good catch, now it should indeed be fixed :)

[chaoran-chen]

Looks great! And I like the refactoring, it's indeed easier to understand!