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.

Sign up for GitHub

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

Jump to bottom

feat(website): Add new form to submit individual sequences #3710

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

feat(website): Add new form to submit individual sequences #3710

wants to merge 69 commits into from
+1,476 −672

Conversation

fhennig
Copy link
Contributor

@fhennig fhennig commented Feb 18, 2025 •
edited
Loading

resolves #3226
resolves #3722

preview URL: https://submission-via-form.loculus.org/dummy-organism/submission/2/submit?inputMode=form

updated docs page: https://docs-submission-via-form.loculus.org/for-users/submit-sequences/

Summary

  • Adds a new inputMode URL param to the submit endpoint with values form and bulk. If it is omitted, we default back to the 'bulk' variant.
  • There is a tab-style selector for bulk vs form submission (see screenshot below)
  • Fixed a mini hydration bug where a td was nested in a div
  • Drop mention of specific data use terms from Acknowledgement section ('Revise' page incorrectly states open data use terms. #3722)
  • Small improvement also for the edit form: internally generated TSV file now uses a library with escaping etc instead of pure string interpolation.

Tests

  • Add new tests for SubmissionRouteUtils
  • Added new tests for the input form (FormOrUploadWrapper.spec.tsx). Test that errors are raised when files or form input is missing. Test that all of the form that gets included in the TSV and also that escaping works.
  • Expanded SubmissionForm tests - submit not only via files, but also via form for some cases.
  • New integration test that submits a sequence via the form

Implementation notes

  • The Edit form as well as the file upload section of the submission form have been extracted into dedicated components
  • A new FormOrUploadWrapper wraps both and makes it easy to switch between them, returning just files which can then be used for uploading
  • Submission form allows manually entering a Submission ID - this is implemented by allowing all noEdit fields to be in fact edited on initial submission - AFAICT this was only used for submission ID and accession (only the submission ID is shown in the 'submit' form).

Screenshot

image

PR Checklist

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

@fhennig fhennig added the preview Triggers a deployment to argocd label Feb 18, 2025
@fhennig fhennig self-assigned this Feb 18, 2025
@fhennig fhennig marked this pull request as draft February 18, 2025 10:36
@fhennig fhennig force-pushed the submission-via-form branch 2 times, most recently from 6e00612 to 5dd4bf5 Compare February 20, 2025 09:48
@fhennig fhennig force-pushed the submission-via-form branch from 1a4178f to f52fc56 Compare February 21, 2025 13:00
@rneher
Copy link

rneher commented Feb 23, 2025

just quickly tested this. I think that it is a bit surprising to land on the submission form for a single sequence by default. I would keep the default as the file upload and suggest that prominent button something with something like "Single sequence submission via form" takes you to the form.

@rneher
Copy link

rneher commented Feb 23, 2025

also. looks like the fields don't accept long enough entries
image

Probably better as a wrapping text field.

@theosanderson
Copy link
Member

also. looks like the fields don't accept long enough entries

I couldn't reproduce this - my theory is it must have been about some field rather than the sequences

Probably better as a wrapping text field.

These text fields for sequences are definitely really problematic. I mostly of think they should in the more final version be file upload components (for the one segment). Copying and pasting sequences feels generally bad practice?

@theosanderson
Copy link
Member

I made some UX suggestions at https://subform2.loculus.org/cchf/submission/1/submit (username: superuser, pw: superuser) for some future version of this (some after this PR), which I've also mentioned on Slack.

@rneher
Copy link

rneher commented Feb 24, 2025

also. looks like the fields don't accept long enough entries

I couldn't reproduce this - my theory is it must have been about some field rather than the sequences

Definitely reproducible for me in CCHF

@rneher
Copy link

rneher commented Feb 24, 2025

Might have been because my sequences included a fasta header. Seems to work without fasta header.

@fhennig fhennig force-pushed the submission-via-form branch from 74a7377 to 5b00d6a Compare February 24, 2025 10:20
@fhennig
Copy link
Contributor Author

fhennig commented Feb 24, 2025 •
edited
Loading

Thanks for your input! I quite like your suggestions Theo, I think I'd maybe just merge them in?

For the sequence fields, maybe we can look into doing already client-side validation, so the users can't submit incorrect characters - or at least, showhow show a better error message.

@theosanderson
Copy link
Member

theosanderson commented Feb 24, 2025 •
edited
Loading

Might have been because my sequences included a fasta header. Seems to work without fasta header.

OK yeah so I think what's happening here is that the whole sequence is being interpreted as the FASTA header

My own preferred way forward on this (and related issues) would be to create a new high priority issue to change all these sequence input text-boxes to upload-a-fasta-file-with-a-single-sequence components.

@fhennig fhennig force-pushed the submission-via-form branch from 9346e69 to 0aaf16b Compare February 24, 2025 14:37
@fhennig fhennig mentioned this pull request Feb 24, 2025
Open
@fhennig
Copy link
Contributor Author

fhennig commented Feb 24, 2025

Since this PR is already quite large, I have made an issue for follow up changes to the form itself: #3740

I want to keep this PR to just the initial creation of the single-sequence submission feature, and the refactoring of the internal components.

IMO it'll be easier to review that way.

@fhennig fhennig force-pushed the submission-via-form branch 2 times, most recently from 1a26723 to 7d81b49 Compare February 25, 2025 10:04
@fhennig fhennig marked this pull request as ready for review February 25, 2025 10:05
@fhennig fhennig force-pushed the submission-via-form branch from 7d81b49 to 167f747 Compare February 25, 2025 11:33
fhennig and others added 28 commits February 26, 2025 15:39
@fhennig
Make form the default and add integration test
7e7172e
@fhennig
foo
a69b4e9
@fhennig
Test fix
a4e978d
@fhennig
Remove broken (and not really necessary) test
4690ee5
@fhennig
make 'form' the default input mode
e19f840
@fhennig
fix website tests
af0ea7a
@fhennig
Default to bulk
f31ce26
@fhennig
adapt integration test
1ccc0f4
@fhennig
Use papaparse instead of string interpolation for TSV building
b2a27b7
@fhennig
Add tests to check TSV contents
c76b2a5
@fhennig
Let users enter a Submission ID
60fc0e1
@fhennig
Better submission ID handling
ebd1031
@fhennig
remove justify-end
3bf3377
@fhennig
Fix website tests
378c6a0
@fhennig
Use tabs
8240333
@fhennig
use default input mode from portal
8e3f1fb
@fhennig
add Submission ID to integration test (should fix)
aea15b0
@fhennig
update docs
f8db899
@fhennig
use const
e4a2073
@fhennig
Add waiting for header to test
a2076f3
@fhennig
formatting
989135f
@fhennig
Increase timeout
7e9fb1b
@fhennig @theosanderson
Update docs/src/content/docs/for-users/submit-sequences.md
131a274 
Co-authored-by: Theo Sanderson <[email protected]>
@fhennig @theosanderson
Update docs/src/content/docs/for-users/submit-sequences.md
a0c6824 
Co-authored-by: Theo Sanderson <[email protected]>
@fhennig
Fix and expand docs
1c7a2ab
@fhennig
Add missing step to submission instructions
6c2e0b7
@fhennig
format
b9636bb
@fhennig
Add some spacing
ab14392
@fhennig fhennig force-pushed the submission-via-form branch from 5f55329 to ab14392 Compare February 26, 2025 15:39
@fhennig
Copy link
Contributor Author

fhennig commented Feb 26, 2025

I'm not gonna merge right away, I'll let it sit at least until the end of tomorrow. Thanks a lot for the review Theo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theosanderson theosanderson theosanderson approved these changes

@chaoran-chen chaoran-chen Awaiting requested review from chaoran-chen

@corneliusroemer corneliusroemer Awaiting requested review from corneliusroemer

Assignees

@fhennig fhennig

Labels
preview Triggers a deployment to argocd
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

'Revise' page incorrectly states open data use terms. Submit individual sequences via a form
3 participants
@fhennig @rneher @theosanderson