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!: Allow disabling data use terms #3468

Open
wants to merge 34 commits into
base: main
Choose a base branch
from
Open

feat!: Allow disabling data use terms #3468

wants to merge 34 commits into from

Conversation

fhennig
Copy link
Contributor

@fhennig fhennig commented Dec 18, 2024

BREAKING values.yaml changes (see below)

resolves #3127

preview URL: https://disable-dut.loculus.org

Summary

This PR makes it possible to disable data use terms (DUT) for the whole Loculus instance with a switch in the values.yaml. This disables DUT-related features and components in various places in the front- and backend (outline below). In the database, all submissions are treated as OPEN.

The new config setting is dataUseTerms.enabled(boolean). It is currently required. (which is breaking because you need to add it and set it to either true or false).

Furthermore, dataUseTermsUrls have been moved to dataUseTerms.urls (breaking).

Full config example:

dataUseTerms:
  enabled: true
  urls:
    open: https://#TODO-MVP/open
    restricted: https://#TODO-MVP/restricted

Frontend changes:

  • DUT elements removed on the submit dialog.
  • 'bulk edit data use terms' removed on the 'released' page.
  • DUT note removed on the API documentation page.
  • DUT elements removed from the Download dialog.

Backend changes:

  • When DUT are disabled, they do not need to be submitted to the submit endpoint
  • When DUT are disabled, the DUT-related fields will not be returned by the get-released-data endpoint.

Testing

  • Frontend
    • Added a test that submission is possible when DUT is disabled
    • Added tests to check that the Download dialog doesn't have the DUT-components
  • backend - add a new config to load, disabling the data use terms
    • Add a test that the DUT related things are not returned by get-released-data
    • Test that the submit endpoint can be called without the DUT type now (if disabled) and test that an error is raised if it is omitted but is required.
  • e2e/integration: Since we cannot dynamically change the values.yaml config, we cannot do any tests at that level.

Screenshot

The submission dialog without the DUT elements:
image

The download dialog without the DUT elements:
image

The API docs don't mention the DUT anmore:
image

New docs page:
image

PR Checklist

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

Discussion about config structure: https://loculus.slack.com/archives/C05G172HL6L/p1738166112981629

@fhennig
Copy link
Contributor Author

fhennig commented Dec 18, 2024

As was discussed earlier, here are the code places marked where I think changes will be introduced to implement this feature.

@corneliusroemer you wanted to have a look first. Putting this on hold till then.

cc @chaoran-chen

@theosanderson
Copy link
Member

Would the backend not need any changes?

@fhennig
Copy link
Contributor Author

fhennig commented Dec 18, 2024

That wasn't in the ticket I think. But maybe that was a mistake? I remember that we talked about that too actually. I'd be curious to look into how to do that

@theosanderson
Copy link
Member

My guess (maybe wrong) of what would happen if you made the changes annotated would be:

  • backend would still expect data use terms on submission so would give an error
  • backend would still emit data use terms in get-released-data so SILO would get confused at the unexpected field

@chaoran-chen
Copy link
Member

We need to do some changes so that (1) the backend accepts submissions that don't provide a data use term and (2) don't expose it in get-released-data. But if it make things easier, it could write "open" into the database.

@fhennig
Copy link
Contributor Author

fhennig commented Dec 18, 2024

Ah, makes sense

@fhennig
Copy link
Contributor Author

fhennig commented Dec 18, 2024

that should cover it.

@fhennig
Copy link
Contributor Author

fhennig commented Jan 29, 2025

I tried quite some time to get the "Submit" button to move to the end of the page, but I couldn't do it without making changes to the BaseLayout and so I'll just leave it for now. If you think it's important then I'll spend some more time on it, but maybe we can also leave it.

image

@theosanderson
Copy link
Member

(I actually don't like huge gaps between the submit button and everything else!)

@fhennig
Copy link
Contributor Author

fhennig commented Jan 29, 2025

perfect! 😄

@fhennig fhennig added the preview Triggers a deployment to argocd label Jan 29, 2025
@fhennig fhennig self-assigned this Jan 29, 2025
@fhennig
Copy link
Contributor Author

fhennig commented Feb 3, 2025

These tests failed:

pages/review/index.spec.ts:53:5 › The review page › approve restricted sequences ──
pages/revise/index.spec.ts:6:5 › The revise page › should upload files and revise existing data 
pages/search/index.spec.ts:135:5 › The search page › should download file when agreeing to terms 
pages/search/index.spec.ts:144:5 › The search page › should download raw nucleotide sequences when selected 
pages/submission/index.spec.ts:14:5 › The submit page › should upload files and submit 
pages/submission/index.spec.ts:29:5 › The submit page › should upload compressed files and submit 
pages/submission/index.spec.ts:42:5 › The submit page › should set data use terms ─

That is because the DUT UI elements are involved. But the tests will work again if the DUTs are enabled again in the values.yaml.

It's unfortunate that our testing setup/the feature being for the whole instance doesn't allow us to test this better (or am I missing something @fengelniederhammer )

Next I'd add some more component tests and backend unit tests.

@fhennig
Copy link
Contributor Author

fhennig commented Feb 4, 2025

e2e test failure is expected, we have to re-enable DUT before merging. (obv. the e2e tests should then be green). I'm leaving it set to 'false' so people can check the preview.

@fhennig fhennig marked this pull request as ready for review February 4, 2025 14:21
@fhennig fhennig changed the title feat: Allow disabling data use terms feat!: Allow disabling data use terms Feb 4, 2025
open: https://#TODO-MVP/open
restricted: https://#TODO-MVP/restricted
dataUseTerms:
enabled: false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
enabled: false
enabled: true

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.

Disable data use terms
3 participants