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(website): review pages are now per-group #1586

Merged
merged 2 commits into from
Apr 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ data class AccessionVersionsFilterWithDeletionScope(
description = ACCESSION_VERSIONS_FILTER_DESCRIPTION,
)
val accessionVersionsFilter: List<AccessionVersion>? = null,
val groupIdsFilter: List<Int>? = null,
@Schema(
description = "Scope for deletion. If scope is set to 'ALL', all sequences are deleted. " +
"If scope is set to 'PROCESSED_WITH_ERRORS', only processed sequences with errors are deleted. " +
Expand All @@ -81,6 +82,7 @@ data class AccessionVersionsFilterWithApprovalScope(
description = ACCESSION_VERSIONS_FILTER_DESCRIPTION,
)
val accessionVersionsFilter: List<AccessionVersion>? = null,
val groupIdsFilter: List<Int>? = null,
@Schema(
description = "Scope for approval. If scope is set to 'ALL', all sequences are approved. " +
"If scope is set to 'WITHOUT_WARNINGS', only sequences without warnings are approved.",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,7 @@ class SubmissionController(
): List<AccessionVersion> = submissionDatabaseService.approveProcessedData(
authenticatedUser = authenticatedUser,
accessionVersionsFilter = body.accessionVersionsFilter,
groupIdsFilter = body.groupIdsFilter,
organism = organism,
scope = body.scope,
)
Expand Down Expand Up @@ -321,6 +322,7 @@ class SubmissionController(
): List<AccessionVersion> = submissionDatabaseService.deleteSequenceEntryVersions(
body.accessionVersionsFilter,
authenticatedUser,
body.groupIdsFilter,
organism,
body.scope,
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -276,9 +276,24 @@ class SubmissionDatabaseService(
)
}

private fun getGroupCondition(groupIdsFilter: List<Int>?, authenticatedUser: AuthenticatedUser): Op<Boolean> {
return if (groupIdsFilter != null) {
groupManagementPreconditionValidator.validateUserIsAllowedToModifyGroups(
groupIdsFilter,
authenticatedUser,
)
SequenceEntriesView.groupIsOneOf(groupIdsFilter)
} else if (authenticatedUser.isSuperUser) {
Op.TRUE
} else {
SequenceEntriesView.groupIsOneOf(groupManagementDatabaseService.getGroupIdsOfUser(authenticatedUser))
}
}

fun approveProcessedData(
authenticatedUser: AuthenticatedUser,
accessionVersionsFilter: List<AccessionVersion>?,
groupIdsFilter: List<Int>?,
organism: Organism,
scope: ApproveDataScope,
): List<AccessionVersion> {
Expand Down Expand Up @@ -315,8 +330,10 @@ class SubmissionDatabaseService(
Op.TRUE
}

val groupCondition = getGroupCondition(groupIdsFilter, authenticatedUser)

val accessionVersionsToUpdate = SequenceEntriesView
.select { statusCondition and accessionCondition and scopeCondition }
.select { statusCondition and accessionCondition and scopeCondition and groupCondition }
.map { AccessionVersion(it[SequenceEntriesView.accessionColumn], it[SequenceEntriesView.versionColumn]) }

if (accessionVersionsToUpdate.isEmpty()) {
Expand Down Expand Up @@ -448,17 +465,7 @@ class SubmissionDatabaseService(

val listOfStatuses = statusesFilter ?: Status.entries

val groupCondition = if (groupIdsFilter != null) {
groupManagementPreconditionValidator.validateUserIsAllowedToModifyGroups(
groupIdsFilter,
authenticatedUser,
)
SequenceEntriesView.groupIsOneOf(groupIdsFilter)
} else if (authenticatedUser.isSuperUser) {
Op.TRUE
} else {
SequenceEntriesView.groupIsOneOf(groupManagementDatabaseService.getGroupIdsOfUser(authenticatedUser))
}
val groupCondition = getGroupCondition(groupIdsFilter, authenticatedUser)

val baseQuery = SequenceEntriesView
.join(
Expand Down Expand Up @@ -611,6 +618,7 @@ class SubmissionDatabaseService(
fun deleteSequenceEntryVersions(
accessionVersionsFilter: List<AccessionVersion>?,
authenticatedUser: AuthenticatedUser,
groupIdsFilter: List<Int>?,
organism: Organism,
scope: DeleteSequenceScope,
): List<AccessionVersion> {
Expand Down Expand Up @@ -655,9 +663,11 @@ class SubmissionDatabaseService(
DeleteSequenceScope.ALL -> SequenceEntriesView.statusIsOneOf(listOfDeletableStatuses)
}

val groupCondition = getGroupCondition(groupIdsFilter, authenticatedUser)

val sequenceEntriesToDelete = SequenceEntriesView
.slice(SequenceEntriesView.accessionColumn, SequenceEntriesView.versionColumn)
.select { accessionCondition and scopeCondition }
.select { accessionCondition and scopeCondition and groupCondition }
.map {
AccessionVersion(
it[SequenceEntriesView.accessionColumn],
Expand Down
27 changes: 25 additions & 2 deletions website/src/components/ReviewPage/ReviewPage.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { describe, expect, test } from 'vitest';

import { ReviewPage } from './ReviewPage.tsx';
import { openDataUseTerms } from '../../../tests/e2e.fixture.ts';
import { mockRequest, testAccessToken, testConfig, testOrganism } from '../../../vitest.setup.ts';
import { mockRequest, testAccessToken, testConfig, testGroups, testOrganism } from '../../../vitest.setup.ts';
import {
approvedForReleaseStatus,
awaitingApprovalStatus,
Expand All @@ -13,9 +13,16 @@ import {
type SequenceEntryStatus,
} from '../../types/backend.ts';

const testGroup = testGroups[0];

function renderReviewPage() {
return render(
<ReviewPage organism={testOrganism} accessToken={testAccessToken} clientConfig={testConfig.public} />,
<ReviewPage
group={testGroup}
organism={testOrganism}
accessToken={testAccessToken}
clientConfig={testConfig.public}
/>,
);
}

Expand Down Expand Up @@ -96,6 +103,22 @@ describe('ReviewPage', () => {
});
});

test('should request data from the right group', async () => {
let requestedGroupFilter: string | null = null;
mockRequest.backend.getSequences(200, generateGetSequencesResponse([]), (request) => {
const params = new URL(request.url).searchParams;
requestedGroupFilter = params.get('groupIdsFilter');
});

const { getByText } = renderReviewPage();

await waitFor(() => {
expect(getByText('You do not currently have any unreleased sequences awaiting review.')).toBeDefined();
});

expect(requestedGroupFilter).toBe(testGroup.groupId.toString());
});

test('should render the review page and show button to bulk delete/approve all erroneous sequences', async () => {
mockRequest.backend.getSequences(
200,
Expand Down
11 changes: 9 additions & 2 deletions website/src/components/ReviewPage/ReviewPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
deleteAllDataScope,
deleteProcessedDataWithErrorsScope,
type GetSequencesResponse,
type Group,
hasErrorsStatus,
inProcessingStatus,
type PageQuery,
Expand All @@ -34,6 +35,7 @@ let oldSequenceData: GetSequencesResponse | null = null;
type ReviewPageProps = {
clientConfig: ClientConfig;
organism: string;
group: Group;
accessToken: string;
};

Expand Down Expand Up @@ -66,12 +68,12 @@ const NumberAndVisibility = ({
);
};

const InnerReviewPage: FC<ReviewPageProps> = ({ clientConfig, organism, accessToken }) => {
const InnerReviewPage: FC<ReviewPageProps> = ({ clientConfig, organism, group, accessToken }) => {
const { errorMessage, isErrorOpen, openErrorFeedback, closeErrorFeedback } = useErrorFeedbackState();

const [pageQuery, setPageQuery] = useState<PageQuery>({ page: 1, size: pageSizeOptions[2] });

const hooks = useSubmissionOperations(organism, clientConfig, accessToken, openErrorFeedback, pageQuery);
const hooks = useSubmissionOperations(organism, group, clientConfig, accessToken, openErrorFeedback, pageQuery);

const showErrors = hooks.includedStatuses.includes(hasErrorsStatus);
const showUnprocessed =
Expand Down Expand Up @@ -229,6 +231,7 @@ const InnerReviewPage: FC<ReviewPageProps> = ({ clientConfig, organism, accessTo
'Are you sure you want to discard all sequences with errors?',
onConfirmation: () => {
hooks.deleteSequenceEntries({
groupIdsFilter: [group.groupId],
scope: deleteProcessedDataWithErrorsScope.value,
});
},
Expand All @@ -248,6 +251,7 @@ const InnerReviewPage: FC<ReviewPageProps> = ({ clientConfig, organism, accessTo
dialogText: `Are you sure you want to discard all ${finishedCount} processed sequences?`,
onConfirmation: () => {
hooks.deleteSequenceEntries({
groupIdsFilter: [group.groupId],
scope: deleteAllDataScope.value,
});
},
Expand All @@ -270,6 +274,7 @@ const InnerReviewPage: FC<ReviewPageProps> = ({ clientConfig, organism, accessTo
dialogText: 'Are you sure you want to release all valid sequences?',
onConfirmation: () =>
hooks.approveProcessedData({
groupIdsFilter: [group.groupId],
scope: approveAllDataScope.value,
}),
})
Expand All @@ -293,12 +298,14 @@ const InnerReviewPage: FC<ReviewPageProps> = ({ clientConfig, organism, accessTo
approveAccessionVersion={() =>
hooks.approveProcessedData({
accessionVersionsFilter: [sequence],
groupIdsFilter: [group.groupId],
scope: approveAllDataScope.value,
})
}
deleteAccessionVersion={() =>
hooks.deleteSequenceEntries({
accessionVersionsFilter: [sequence],
groupIdsFilter: [group.groupId],
scope: deleteAllDataScope.value,
})
}
Expand Down
3 changes: 3 additions & 0 deletions website/src/hooks/useSubmissionOperations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { backendApi } from '../services/backendApi.ts';
import { backendClientHooks } from '../services/serviceHooks.ts';
import {
awaitingApprovalStatus,
type Group,
hasErrorsStatus,
inProcessingStatus,
type PageQuery,
Expand All @@ -17,6 +18,7 @@ import { stringifyMaybeAxiosError } from '../utils/stringifyMaybeAxiosError.ts';

export function useSubmissionOperations(
organism: string,
group: Group,
clientConfig: ClientConfig,
accessToken: string,
openErrorFeedback: (message: string) => void,
Expand All @@ -32,6 +34,7 @@ export function useSubmissionOperations(
organism,
},
queries: {
groupIdsFilter: group.groupId.toString(),
initialStatusesFilter: allRelevantStatuses.join(','),
statusesFilter: includedStatuses.join(','),
page: pageQuery.page - 1,
Expand Down
36 changes: 21 additions & 15 deletions website/src/pages/[organism]/submission/[groupId]/review.astro
Original file line number Diff line number Diff line change
@@ -1,24 +1,30 @@
---
import { ReviewPage } from '../../../../components/ReviewPage/ReviewPage';
import NeedToLogin from '../../../../components/common/NeedToLogin.astro';
import SubmissionPageWrapper from '../../../../components/Submission/SubmissionPageWrapper.astro';
import { getRuntimeConfig } from '../../../../config';
import BaseLayout from '../../../../layouts/BaseLayout.astro';
import { type ClientConfig } from '../../../../types/runtimeConfig';
import { getAccessToken } from '../../../../utils/getAccessToken';
import { getGroupsAndCurrentGroup } from '../../../../utils/submissionPages';

const organism = Astro.params.organism!;
const accessToken = getAccessToken(Astro.locals.session)!;
const groupsResult = await getGroupsAndCurrentGroup(Astro.params, Astro.locals.session);

const clientConfig: ClientConfig = getRuntimeConfig().public;
---

{
accessToken ? (
<BaseLayout title='Current submissions'>
<h1 class='title'>Current submissions</h1>
<ReviewPage clientConfig={clientConfig} organism={organism} accessToken={accessToken} client:load />
</BaseLayout>
) : (
<BaseLayout title='Current submissions'>
<NeedToLogin />
</BaseLayout>
)
}
<SubmissionPageWrapper title='Review current submissions' organism={organism} groupsResult={groupsResult}>
{
groupsResult.match(
({ currentGroup: group }) => (
<ReviewPage
clientConfig={clientConfig}
organism={organism}
group={group}
accessToken={getAccessToken(Astro.locals.session)!}
client:load
/>
),
() => undefined,
)
}
</SubmissionPageWrapper>
2 changes: 2 additions & 0 deletions website/src/types/backend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ export const approveProcessedDataWithoutWarningsScope = z.literal('WITHOUT_WARNI

export const accessionVersionsFilterWithApprovalScope = accessionVersionsFilter.merge(
z.object({
groupIdsFilter: z.array(z.number()),
scope: z.union([approveAllDataScope, approveProcessedDataWithoutWarningsScope]),
}),
);
Expand All @@ -72,6 +73,7 @@ export const deleteProcessedDataWithWarningsScope = z.literal('PROCESSED_WITH_WA

export const accessionVersionsFilterWithDeletionScope = accessionVersionsFilter.merge(
z.object({
groupIdsFilter: z.array(z.number()),
scope: z.union([
deleteAllDataScope,
deleteProcessedAndRevocationConfirmationDataScope,
Expand Down
19 changes: 16 additions & 3 deletions website/tests/util/backendCalls.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,14 @@ export const submitRevisedDataViaApi = async (accessions: Accession[], token: st
return response.value;
};

export const approveProcessedData = async (accessionVersions: AccessionVersion[], token: string): Promise<void> => {
export const approveProcessedData = async (
accessionVersions: AccessionVersion[],
token: string,
groupId: number,
): Promise<void> => {
const body = {
accessionVersionsFilter: accessionVersions,
groupIdsFilter: [groupId],
scope: 'ALL' as const,
};

Expand All @@ -64,7 +69,11 @@ export const approveProcessedData = async (accessionVersions: AccessionVersion[]
}
};

export const revokeReleasedData = async (accessions: Accession[], token: string): Promise<AccessionVersion[]> => {
export const revokeReleasedData = async (
accessions: Accession[],
token: string,
groupId: number,
): Promise<AccessionVersion[]> => {
const body = {
accessions,
};
Expand All @@ -83,7 +92,11 @@ export const revokeReleasedData = async (accessions: Accession[], token: string)

const confirmationResponse = await backendClient.call(
'approveProcessedData',
{ scope: 'ALL', accessionVersionsFilter: accessionVersions },
{
scope: 'ALL',
accessionVersionsFilter: accessionVersions,
groupIdsFilter: [groupId],
},
{
params: { organism: dummyOrganism.key },
headers: createAuthorizationHeader(token),
Expand Down
5 changes: 3 additions & 2 deletions website/tests/util/prepareDataToBe.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ const prepareDataToBeAwaitingApproval = async (token: string, groupId: number) =
const prepareDataToBeApprovedForRelease = async (token: string, groupId: number) => {
const sequenceEntries = await prepareDataToBeAwaitingApproval(token, groupId);

await approveProcessedData(sequenceEntries, token);
await approveProcessedData(sequenceEntries, token, groupId);

return sequenceEntries;
};
Expand All @@ -67,6 +67,7 @@ const prepareDataToBeRevoked = async (token: string, groupId: number) => {
return revokeReleasedData(
sequenceEntries.map((entry) => entry.accession),
token,
groupId,
);
};

Expand All @@ -87,7 +88,7 @@ const prepareDataToBeRevisedForRelease = async (token: string, groupId: number)
}));
await fakeProcessingPipeline.submit(options);

await approveProcessedData(submittedRevisionAccessionVersion, token);
await approveProcessedData(submittedRevisionAccessionVersion, token, groupId);

return submittedRevisionAccessionVersion;
};
Loading
Loading