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

1367 multiple env select #1372

Merged
merged 12 commits into from
Dec 10, 2024
Merged

1367 multiple env select #1372

merged 12 commits into from
Dec 10, 2024

Conversation

marySalvi
Copy link
Collaborator

This is ready for a review but not ready to be merged yet.

closes #1367
Allows the ability to select multiple enviroment extensions f/k/a environment packages.

Testing:

  • Create new study.
  • 'Soil' should be selected when on the environment extension step
  • Should be able to select more extensions
  • When navigating to the DH the corresponding template tabs should be available.

TODO:
Before this can be merged the corresponding runtime code needs to be updated.

@marySalvi marySalvi requested a review from naglepuff August 29, 2024 14:43
@marySalvi marySalvi marked this pull request as draft August 29, 2024 20:41
@marySalvi marySalvi removed the request for review from naglepuff August 29, 2024 20:54
@marySalvi marySalvi marked this pull request as ready for review September 16, 2024 17:40
@marySalvi marySalvi force-pushed the 1367-multiple-env-select branch from 3650877 to b170e6d Compare September 16, 2024 17:49
Copy link
Collaborator

@pkalita-lbl pkalita-lbl left a comment

Choose a reason for hiding this comment

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

I tested this branch a bit locally and found a few issues:

  1. I created a new submission. Once I got to the /templates page, I saw that "soil" was pre-checked, as expected. However I was able to uncheck this box and click the "go to next step" button. I expected that button to be disabled if no checkboxes are checked.
    1. I created a new submission and selected the "soil" and "water" extensions. On the /samples page I see a "soil" tab and a "water" tab as expected.
    2. In the "sample name" column I entered 1, 2, and 3 in the first three rows, respectively, of the "soil" tab.
    3. I clicked on the "water" tab and entered 4, 5, and 6 into the "sample name" column of the first three rows, respectively.
    4. I clicked on the "soil" tab again and saw the first three rows filled out as expected.
    5. I clicked on the "water" tab again and saw the first three rows were blank. I expected to see 4, 5, and 6 in the first column.
    1. I created a new submission and on the /context page I selected "No" for "Have data already been generated for your study?" and "JGI" for "Are you submitting metadata for samples that will be sent to a DOE user facility?" and "CSP" for "What kind of project have you been awarded?".
    2. On the /multiomics page I selected "Metagenome" in the JGI section and entered "123456" as a proposal ID.
    3. On the /templates page I selected "soil" and "water".
    4. On the /samples page I entered 1, 2, and 3 in the first three rows, respectively, of the "sample name" column and selected "metagenomics" for the first three rows in the "analysis/data type" column.
    5. I clicked on the "water" tab and entered 4, 5, and 6 into the first three rows, respectively, of the "sample name" column. I selected "metagenomics" for the first three rows in the "analysis/data type" column.
    6. I clicked on the "JGI MG" tab and saw no rows. I think would have expected to see six rows with 1 through 6 in the first column. I say "think" because I'm not sure the behavior in this case has been clearly defined.

This is mostly a note to myself, but this change will need corresponding changes in the Field Notes app. This will be a good test case for releasing coordinating changes to the backend and the app.

@marySalvi
Copy link
Collaborator Author

    1. I created a new submission and on the /context page I selected "No" for "Have data already been generated for your study?" and "JGI" for "Are you submitting metadata for samples that will be sent to a DOE user facility?" and "CSP" for "What kind of project have you been awarded?".
    2. On the /multiomics page I selected "Metagenome" in the JGI section and entered "123456" as a proposal ID.
    3. On the /templates page I selected "soil" and "water".
    4. On the /samples page I entered 1, 2, and 3 in the first three rows, respectively, of the "sample name" column and selected "metagenomics" for the first three rows in the "analysis/data type" column.
    5. I clicked on the "water" tab and entered 4, 5, and 6 into the first three rows, respectively, of the "sample name" column. I selected "metagenomics" for the first three rows in the "analysis/data type" column.
    6. I clicked on the "JGI MG" tab and saw no rows. I think would have expected to see six rows with 1 through 6 in the first column. I say "think" because I'm not sure the behavior in this case has been clearly defined.

@mslarae13 @aclum Thoughts on correct behavior in this instance? Are there others I should loop in?

@aclum
Copy link
Contributor

aclum commented Sep 18, 2024

you are correct, there should be 6 rows with 1-6

@mslarae13
Copy link

just looking at this but yes. the current behavior with 1 sample type for JGI or EMSL metadata & sample submission (data not generated yet) is to have samples of specific types appear in the respective user facilities sheet.
so if all 6 samples (3 water, 3 soil) have metaGs, they should appear in the JGI tab.

@marySalvi
Copy link
Collaborator Author

@pkalita-lbl I think I have addressed all of the issues/bugs you found. Do you mind checking again?

Copy link
Collaborator

@pkalita-lbl pkalita-lbl left a comment

Choose a reason for hiding this comment

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

I believe everything is working as expected now! I made one minor comment, but I'll mark this as approved so that we can get this on dev soon and other folks can test it.

There's a good chance I might request one or two minor backend tweaks to help ease the transition from the Field Notes perspective. But I think it'll be easier if I just open a separate PR for that later.

Copy link
Collaborator

@naglepuff naglepuff left a comment

Choose a reason for hiding this comment

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

LGTM! Some incredibly nitpicky comments, which you can disregard if you'd like

@mslarae13
Copy link

@pkalita-lbl status on getting this for the app so we can merge?

@marySalvi
Copy link
Collaborator Author

@pkalita-lbl status on getting this for the app so we can merge?

@mslarae13 I will coordinate with Patrick my other changes to runtime to make sure this can get merged either today or tomorrow.

@aclum
Copy link
Contributor

aclum commented Nov 14, 2024

Checking in on this since the last comment was that this would be merged shortly and that was 2 weeks ago. FYI there is currently a conflict in nmdc_server/schemas_submission.py

@marySalvi marySalvi force-pushed the 1367-multiple-env-select branch from 784eba1 to 24a0daf Compare December 6, 2024 22:01
@marySalvi marySalvi requested a review from naglepuff December 6, 2024 22:02
@marySalvi marySalvi changed the title DON"T MERGE 1367 multiple env select 1367 multiple env select Dec 6, 2024
Copy link
Collaborator

@naglepuff naglepuff left a comment

Choose a reason for hiding this comment

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

Tested it out, looks good, I'm just wondering if there's any value in adding a downgrade operation to the new migration.

@marySalvi marySalvi requested a review from naglepuff December 9, 2024 17:56
@marySalvi marySalvi force-pushed the 1367-multiple-env-select branch from c91fd55 to 19fcef5 Compare December 9, 2024 17:59
Copy link
Collaborator

@naglepuff naglepuff left a comment

Choose a reason for hiding this comment

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

LGTM

@marySalvi marySalvi merged commit 73b6f2b into main Dec 10, 2024
2 checks passed
@marySalvi marySalvi deleted the 1367-multiple-env-select branch December 10, 2024 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to select multiple sample type environment
5 participants