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

Do not allow submissionId to be editable #2334

Merged
merged 8 commits into from
Jul 27, 2024
Merged

Do not allow submissionId to be editable #2334

merged 8 commits into from
Jul 27, 2024

Conversation

anna-parker
Copy link
Contributor

@anna-parker anna-parker commented Jul 25, 2024

resolves ##2121

preview URL: https://fix-submitterid.loculus.org/

Summary

Current state: submitter Id field is empty (the contents do appear when metadata is downloaded - so I don't know why this is currently empty) and arbitrary content can be added:

image

There are 2 issues here, the first is that the submitterId is not actually passed to the website from the backend, but also that the submitterId is still in the list of original metadata although the value is not there.

  • Pass submissionId to website from backend
  • Remove submissionId from original, editable metadata list by adding a noEdit field in the config.
  • Add submissionId as a non-editable field
image

Testing

Locally I checked that the submitterId is still returned in the downloaded metadata and is seen on the sequence details page after release:
image

@anna-parker anna-parker added the preview Triggers a deployment to argocd label Jul 25, 2024
@anna-parker anna-parker marked this pull request as ready for review July 25, 2024 12:10
@theosanderson
Copy link
Member

I think your earlier version had this just displayed in a non-editable format? I think that's pretty useful because if the user submitted their sample, which they know only by their own ID Crick_sample_A123 then they won't be able to enter the corresponding metadata without referring to that ID.

@theosanderson
Copy link
Member

I guess you did this because of this empty thing. I can try to look into what's going on there.

@theosanderson
Copy link
Member

Working on: #2336

@theosanderson theosanderson changed the title Do not allow submitterId to be editable Do not allow submissionId to be editable Jul 25, 2024
@theosanderson
Copy link
Member

Interestingly I tried on main to see the submissionId on the edit page for dummy organism but couldn't. Maybe dummy organism is set up differently. (Then I tried ebola, but hit #2339 so couldn't confirm either way)

@anna-parker
Copy link
Contributor Author

@theosanderson thanks for looking into the submitterId - I fixed adding it here to the webpage - will push my changes in a bit!

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.

This is great! Thanks a lot

website/src/components/Edit/EditPage.tsx Outdated Show resolved Hide resolved
@anna-parker anna-parker merged commit 568a34c into main Jul 27, 2024
16 checks passed
@anna-parker anna-parker deleted the fix_submitterId branch July 27, 2024 14:50
@fengelniederhammer
Copy link
Contributor

I wonder why this was necessary at all? We have a bunch of loculus internal metadata, why did this only show up for the submission id? Is it becaus the submission id somehow made it to the values.yaml although it should be only in the _common-metadata.tpl?

@theosanderson
Copy link
Member

Yes I have a feeling that's right and that in fact it was only present for some organisms because I couldn't reproduce this for dummyOrganism (which is the only place I tried)

@fengelniederhammer
Copy link
Contributor

Should we maybe revert this PR and instead fix it in the values.yml (for the sake of complexity)?

@theosanderson
Copy link
Member

No, this PR achieved stuff we need: the submissionId should be displayed but in a non-editable form. The reason it needs to be displayed is that users need to refer to it to look up the correct metadata to enter

@theosanderson
Copy link
Member

(the thing that may not be needed is the noEdit setting, no strong feelings there)

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.

3 participants