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): Change data use terms in bulk #3322

Merged
merged 60 commits into from
Dec 10, 2024
Merged

Conversation

fhennig
Copy link
Contributor

@fhennig fhennig commented Dec 2, 2024

resolves #992

preview URL: http://datausetermsbulkchange.loculus.org

Summary

  • new EditDataUseTermsModal
    • Has 3 different variants, depending on if all selected sequences are OPEN, all RESTRICTED or a mix of both.
    • Displayed only in the 'View your group's released' page
    • fetches data from LAPIS and updates it in the backend
  • Refactored
    • The DateTimePicker is now part of the DataUseTermsSelector component and has been pulled out of the submission form and EditDataUseTermsButton. The DataUseTermsSelector has a setting to switch between an inline and dialog DateTimePicker.
    • The new EditDateUseTermsModal and the DownloadDialog use a common BaseDialog component.
    • The DownloadParameters union type has been refactored into a SequenceFilter that is now also used by the new modal to get the LAPIS query parameters it needs. Various code spread around components has been factored into methods of the new classes.
  • Add a test for the new extended DataUseTermsSelector

Screenshot

There are 3 different types of panels, depending on the selection of sequences. below is the one for when only restricted sequences are selected:
image

PR Checklist

  • All necessary documentation has been adapted. (not applicable?)
  • The implemented feature is covered by an appropriate test. (The DataUseTermsSelector is tested, and then simply emebedded in the new modal. That seemed sufficient to me)

@fhennig fhennig added the preview Triggers a deployment to argocd label Dec 2, 2024
@fhennig

This comment was marked as outdated.

@fhennig

This comment was marked as outdated.

@fhennig

This comment was marked as outdated.

@fhennig fhennig force-pushed the datausetermsbulkchange branch from 21279a9 to 0fa3aef Compare December 3, 2024 16:20
@fhennig

This comment was marked as outdated.

@fhennig

This comment was marked as outdated.

@fhennig

This comment was marked as outdated.

@fhennig fhennig force-pushed the datausetermsbulkchange branch from cfb812b to e549f99 Compare December 4, 2024 09:21
@corneliusroemer
Copy link
Contributor

This is the test that failed:

1 failed
    [chromium] › pages/search/index.spec.ts:61:5 › The search page › should search many sequence entries by accession 
  1 flaky

  1) [chromium] › pages/search/index.spec.ts:61:5 › The search page › should search many sequence entries by accession 

    Error: expect(received).toBeTruthy()

    Received: false

      74 |         expect(newAccessions.length).toBe(3);
      75 |         expect(newAccessions.includes(previousAccessions[0])).toBeTruthy();
    > 76 |         expect(newAccessions.includes(previousAccessions[1])).toBeTruthy();
         |                                                               ^
      77 |         expect(newAccessions.includes(previousAccessions[2])).toBeTruthy();
      78 |     });
      79 |
        at /home/runner/work/loculus/loculus/website/tests/pages/search/index.spec.ts:76:63

@fhennig fhennig force-pushed the datausetermsbulkchange branch from bed5bde to eb9c3a8 Compare December 4, 2024 15:43
@fhennig fhennig self-assigned this Dec 4, 2024
@fhennig fhennig marked this pull request as ready for review December 4, 2024 16:29
@fhennig fhennig changed the title feat: Change data use terms in bulk feat(website): Change data use terms in bulk Dec 4, 2024
@fhennig fhennig force-pushed the datausetermsbulkchange branch from 4f25f44 to f4a688e Compare December 5, 2024 14:00
Copy link
Member

@theosanderson theosanderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really good work, I had a look through all the code and it looks great to my eyes

@fhennig
Copy link
Contributor Author

fhennig commented Dec 6, 2024

Some more feedback I got from @corneliusroemer

  • The two column open/restricted selector doesn't look good on mobile (-> maybe make the layout responsive)
  • One can apply a "non-update" and it shows in the data use terms history (should just not update)
  • There is an error message "Please chose a date before ..." (see image below) and it should say "before or equal to", because once can 'equalize' the dates on multiple sequences.
Image with the error message

image

Thanks Theo and Cornelius for the review!!

@theosanderson
Copy link
Member

To my mind we don't need to worry that much about mobile responsivity for submission-related pages (but ofc. no problem with making it better if that's not too hard)

@fhennig fhennig force-pushed the datausetermsbulkchange branch from dd7860c to cc0a415 Compare December 6, 2024 22:20
@fhennig
Copy link
Contributor Author

fhennig commented Dec 7, 2024

regarding "One can apply a "non-update" and it shows in the data use terms history"

I implemented a fix here: d2e5503 (#3322)

It wasn't possible to do the filtering in the backend without breaking up the batchInsert into individual inserts, and so I decided to calculate the affected accessions in the frontend. This is actually cool, because now the number on the button shows correctly how many sequences actually receive a changed date!

@fhennig fhennig requested a review from theosanderson December 7, 2024 14:05
Copy link
Member

@theosanderson theosanderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't look through all the code again, but functionality seems good now and we've had a pass through the code so I'm approving. Thanks for all the hard work!

@fhennig fhennig enabled auto-merge (squash) December 10, 2024 15:35
@fhennig fhennig merged commit 272d34b into main Dec 10, 2024
19 checks passed
@fhennig fhennig deleted the datausetermsbulkchange branch December 10, 2024 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Triggers a deployment to argocd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change data use term in bulk
3 participants