-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add support for multiple environmental templates #208
Conversation
…g or array of strings
…bility selections
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for implementing this, including coordinating the backend changes. I found the PR description very helpful in preparing my mind with relevant context for reviewing the diff.
I'm comfortable with this being merged in as is. I have one concern that I do not consider to be a deal breaker. I think resolving it would involve making a large quantity of small changes to the diff. That is: to refer to a template name as templateName
instead of as template
. I left some comments on the diff about this.
I have not tested this branch on my system yet. I will do that now. I don't think I'll have time to exercise all the new functionality, but I will do some baseline tests.
_index: number; | ||
_flatIndex: number; | ||
_templateIndex: number; | ||
_template: TemplateName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is among the first two files I've read in the diff so far—I haven't seen most of the diff yet. At this point, I'd prefer that this be named _templateName
, since I think it contains a string representing the name of a template, as opposed to containing the template, itself. I don't know whether I'll still have that preference after I'm done reading the remainder of the diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After having read the rest of the diff, I would prefer the various template
variables that contain template names, be named templateName
; to distinguish them from the object stored in TEMPLATES[templateName]
, which I assume is a "template".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I like the idea. I started doing that and then realized that it would actually be a lot of changes; enough that I'd be worried about breaking things. The changes would start branching out into files not originally touched here. So I think I might save that as a follow-on task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let count = 0; | ||
const templates = getSubmissionTemplates(submission); | ||
templates.forEach((template) => { | ||
const sampleDataSlot = TEMPLATES[template]?.sampleDataSlot; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a follow-up comment to the first one I left. This is the final file in the diff. After having reviewed the other files in the diff, I'd prefer template
be named templateName
. Given what template
contains (i.e. the name of a template), I find TEMPLATES[templateName]
easier to mentally process than TEMPLATES[template]
.
In my local development environment, I failed to test this against the production With that being the case, I will switch to testing against the development
I'm done doing the local testing I planned to do (given my workload today). Looks good! |
Co-authored-by: eecavanna <[email protected]>
Fixes #177
Summary
These changes add support for selecting multiple environmental templates. This is enabled by backend support provided by microbiomedata/nmdc-server#1372 and microbiomedata/nmdc-server#1491.
Because we can't 100% control the timing of the app's release relative to the backend's release, the overall approach of these changes is to make the app agnostic to the
packageName
field being a single string (the old single template format) or an array of strings (the new multiple template format).The one thing that the app can't know a priori is whether it should send a single string or an array of strings when creating a new submission (if you're working with an existing submission the app will just keep whatever it was given -- string or array -- by the backend). The one place that's controlled is in
src/data.ts
and that's what we'll need to change once the backend changes are on production and we're ready to have multiple environment selection be the default.Details
packageName
as a string vs an array of strings is encapsulated in new function insrc/utils.ts
.getSubmissionTemplates
is new and returns an array of template names regardless of whether the submission'spackageName
field is a string or an array of strings.getSubmissionSamples
previously returned a list of sample metadata objects. Now it returns an object where each key is a template name and the value is a list of sample metadata objects for that template.getSubmissionSamplesCount
adds up the number of sample metadata objects across all of the templates.getSubmissionSamplesForTemplate
is new and is mainly a shortcut for indexing into the object returned bygetSubmissionSamples
.getSubmissionSample
previously only needed an numeric index to identify a single sample within a submission. Now it needs both a template name and a numeric index.SamplePage
now requires atemplate
parameter. This is also reflected in the routing to this page (see:src/Router.tsx
,src/paths.ts
,src/components/SampleList/SampleList.tsx
)SampleList
component still displays all samples in a single flat list. This means it needs to do a little extra work to keep track of a "flattened index" for the purpose of uniquely identifying samples in that list. It also needs to ask the user which template to use when creating a new sample (if more than one template is associated with the study). And finally theIonChip
elements it presents are now interactive, as a way to filter the samples by template.src/components/StudyForm/StudyForm.tsx
) there was existing logic to keep two backend data fields (packageName
andtemplates
) in sync (arguably the backend should do this automatically, but it doesn't for reasons perhaps lost to history). This logic got a bit more complex in order to handle both plain strings and arrays of strings.StudyView
(src/components/StudyView/StudyView.tsx
) has been updated so that it automatically shows the slot selector modal for each template where the user has yet to choose visible slots for that template. This actually removes a bit of the logic based on routing state introduced in Store slot visibility settings on a per-Study basis #206. After thinking about it more, I realized we should probably just prompt the user to choose their visible slots if they haven't done so already no matter how they landed on that page. Mainly I'm thinking about the use case where someone creates a submission in the Submission Portal and then starts working with that submission in the app. We wouldn't want them to miss the slot visibility selection step just because they didn't create the submission in the app.Testing
The most important thing is that these change should work while talking to a backend running the latest
nmdc-server
changes (i.e. running themain
branch locally or pointing todata-dev.microbiomedata.org
) or the current productionnmdc-server
version (i.e. running thev1.1.1
tag locall or pointing todata.microbiomedata.org
).Using either backend you should be able to create new submissions, edit existing ones, and add samples to a submission.
You will see slightly different behaviors when using the dev vs. prod backends. When using dev you should see the templates explicitly listed on the study view page, and tapping on one should bring up the slot selection modal. You will not see on that prod because that version of the backend does not support it yet.