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

Feature/linking accession ids #330

Merged
merged 15 commits into from
Jun 24, 2021
Merged

Conversation

hannyle
Copy link
Contributor

@hannyle hannyle commented Jun 2, 2021

Description

Related issues

#58

Type of change

  • New feature (non-breaking change which adds functionality)

Changes Made

  • Modify dereferencedSchema in WizardFillObjectDetailsForm to add arrays of accessionId of other ObjectTypes
  • Add e2e test for testing linking accessionIds of all ObjectTypes
  • Update unit test

Testing

  • Unit Tests
  • Integration Tests

Mentions

  • In Analysis form, there is a section for Analysis Reference and inside it there is a field for Accession Id. This could mean that Analysis could link to each other (?)
  • Users may confuse when they see only the accessionIds without knowing exactly which submission is which. We can add the title there for ones that have title.

@hannyle hannyle self-assigned this Jun 2, 2021
@hannyle hannyle added the enhancement New feature or request label Jun 2, 2021
@hannyle hannyle added this to the Beta - User Interaction milestone Jun 2, 2021
@hannyle hannyle linked an issue Jun 2, 2021 that may be closed by this pull request
Copy link
Contributor

@saulipurhonen saulipurhonen left a comment

Choose a reason for hiding this comment

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

Is it on purpose with FieldArray fields to automatically choose reference accession ID when clicking Add new item?
For example in Run > Experiment Reference

If selection field is visible on initial render, default value is empty. Eg. Experiment > Study Reference

I think it's ok to work this way and if we want to change it, we can do it afterwards.

Otherwise LGTM!

@hannyle
Copy link
Contributor Author

hannyle commented Jun 3, 2021

FieldArray fields to automatically choose reference accession ID when clicking Add new item

--> This is not on purpose, a bug actually. I think it shouldn't render any selection in the beginning. It can be fixed in another PR.

@blankdots
Copy link
Contributor

Looks good! however the accession id might be something that users might find difficult to identify.
Is there a way to also put in the drop down the title near the id - but when it is selected just put in the field the accession id.
something like:

drop down 🔽
359625734 (study 1)
589675476 (study 2)

When user selects 589675476 (study 2) in the field only 589675476 remains

This is not mandatory and could be part of another PR. What do you think?

cypress/integration/app.spec.js Outdated Show resolved Hide resolved
@norling
Copy link

norling commented Jun 15, 2021

Hi!

I did some limited testing, and had some trouble.
To replicate, create a new submission:

  • create and submit a sample
  • click "Experiment"->"Fill Form"

The site crashes with:

Uncaught (in promise) TypeError: s.filter is not a function         WizardJSONSchemaParser.js:356:39
    children WizardJSONSchemaParser.js:356
    _r WizardJSONSchemaParser.js:128
    React 7
    unstable_runWithPriority scheduler.production.min.js:18
    React 5
    e WizardFillObjectDetailsForm.js:466
    l runtime.js:63
    _invoke runtime.js:293
    S runtime.js:118
    Babel 2

Tested in firefox 89 and chrome 91.0.4472.101.

@hannyle
Copy link
Contributor Author

hannyle commented Jun 16, 2021

Looks good! however the accession id might be something that users might find difficult to identify.
Is there a way to also put in the drop down the title near the id - but when it is selected just put in the field the accession id.
something like:

drop down 🔽
359625734 (study 1)
589675476 (study 2)

When user selects 589675476 (study 2) in the field only 589675476 remains

This is not mandatory and could be part of another PR. What do you think?

This is a good idea. We can put the title next to AccessionId.
I also think that in some of submitted forms, they may not have the title because the title is not required, then we can only show the AccessionId. Or with the case of DAC form, we then need to show the title as Main Contact: Name.

@hannyle
Copy link
Contributor Author

hannyle commented Jun 17, 2021

Hi!

I did some limited testing, and had some trouble.
To replicate, create a new submission:

  • create and submit a sample
  • click "Experiment"->"Fill Form"

The site crashes with:

Uncaught (in promise) TypeError: s.filter is not a function         WizardJSONSchemaParser.js:356:39
    children WizardJSONSchemaParser.js:356
    _r WizardJSONSchemaParser.js:128
    React 7
    unstable_runWithPriority scheduler.production.min.js:18
    React 5
    e WizardFillObjectDetailsForm.js:466
    l runtime.js:63
    _invoke runtime.js:293
    S runtime.js:118
    Babel 2

Tested in firefox 89 and chrome 91.0.4472.101.

Hi! I hope my latest commit would fix this issue.

@blankdots blankdots merged commit f5eec06 into develop Jun 24, 2021
@blankdots blankdots deleted the feature/linking-accessionIds branch June 24, 2021 05:52
@blankdots blankdots mentioned this pull request Aug 11, 2021
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

figure out linking of accession IDs
5 participants