loculus-project / loculus

An open-source software package to power microbial genomic databases
https://loculus.org
GNU Affero General Public License v3.0
37 stars 2 forks source link

feat: support filtering for warnings only on the review page #3125

Closed fhennig closed 1 week ago

fhennig commented 3 weeks ago

part of #3031

fixes #2566 fixes #1447 fixes #3198 -- ac6aa79

preview URL: https://feat-3031-improve-review.loculus.org/

Summary

This PR merges the HAS_ERRORS and AWAITING_APPROVAL statuses in the SequenceEntryView. Instead errors are handled by looking at the errors column. The get-sequences endpoint now supports filtering by processing result (no_issues, warnings, errors) and also returns the counts for these states.

The UI has been adapted to support filtering.

Documentation has been updated.

The PR also adds confirmation dialogs to individual sequence publishing or deletion (#2566)

Screenshot

image

PR Checklist

theosanderson commented 3 weeks ago

Huh interesting the migration SQL CI thing may not be functioning as we expected, apologies. Away for a bit but will look into it.

fhennig commented 3 weeks ago

Hmm, I see e2e tests fail on main I think? I'm not sure the e2e test failures are related to my changes in this PR

fhennig commented 2 weeks ago

I have now moved the processing_result into a column in the view, so it's not calculated in the backend code anymore. I have also updated the preprocessing status field to not include FINISHED and HAS_ERRORS anymore.

TODO: still need to update the frontend

fhennig commented 2 weeks ago

Alright! Thanks for @corneliusroemer and @anna-parker pointing out that there wasn't any migration for the processing_status column, I have fixed that now!

I have tested the migration locally by first deploying the DB from main, and then manually migrating it and deploying the new frontend and backend, and it was working for me.

corneliusroemer commented 2 weeks ago

Thanks @fhennig, did you ensure that there was some data in the db? How long was the main deployment up and had ingest run at least partially? If there's nothing processed it could be a false positive. Easiest might actually to test this on staging with prod data. We should probably do this before merging in case fixes are necessary. And given the wide ranging changes there's a non-zero chance we'll hit some edge case that's hard to get with test data.

corneliusroemer commented 2 weeks ago

Need to fix the merge conflicts before we can test this on staging unfortunately - when merge conflicts exist the CI doesn't run.

The CI runs on the outcome of the eventual merge, so with conflict it can't run.

corneliusroemer commented 2 weeks ago

Great work, just reporting my staging.ppx.bio test results (where rollout worked!):

I think I found a small bug, if you uncheck errors, one still gets shown a lot of pages to page through, even though there are 0 results.

Google Chrome Beta 2024-11-11 19 15 10

Another smallish one: the group dropdown is behind the new filters, some z-score thing:

Google Chrome Beta 2024-11-11 19 17 25

2024-11-11 19 17 34

theosanderson commented 2 weeks ago

if you uncheck errors, one still gets shown a lot of pages to page through, even though there are 0 results.

Nice catch. I just checked and this is also on main - so not new - but still would be great to fix here!

theosanderson commented 2 weeks ago

Another smallish one: the organism dropdown is behind the new filters, some z-score thing:

This is the group selector [as I know you know :) ] also most likely a pre-existing issue - it may not be worth blocking on it given multiple groups will be rare, but we should definitely try to fix it pretty soon

corneliusroemer commented 2 weeks ago

Indeed both are on prod, will make 2 issues and hide it here. Thanks @theosanderson

fhennig commented 2 weeks ago

I fixed the pagination bit in this commit: cddfe83

Ideally the number of results would maybe be returned in the get-sequences response, but I think this is fine for now.

corneliusroemer commented 1 week ago

I wonder if this change should be reverted: https://github.com/loculus-project/loculus/pull/3125#discussion_r1846874164 until we have made it in code