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

Bugfix/edit does not work for all fields #323

Merged
merged 7 commits into from
Jun 24, 2021

Conversation

saulipurhonen
Copy link
Contributor

Description

Some fields didn't populate correctly when editing saved / submitted form.

Related issues

Closes #256 and #295

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Changes Made

  • Set default value for OneOf -field if there's a selection in object data
  • Set checked status for Checkbox component
  • Dedicated E2E tests for form editing

Testing

  • E2E Tests

@saulipurhonen saulipurhonen added the bug Something isn't working label May 28, 2021
@saulipurhonen saulipurhonen self-assigned this May 28, 2021
Copy link
Contributor

@hannyle hannyle left a comment

Choose a reason for hiding this comment

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

I can see that the issue with Sample form has been fixed.
However for the other forms: Analysis and Run, the forms still miss some nested fields when editing.

For example: in Analysis:

  1. Select Reference Alignment for Analysis Type
  2. Select Standard for Reference assembly details
  3. Fill in the field Accession Id of Standard
  4. Click Save as Draft
  5. Edit this draft
  6. The fields append to Standard is missing

@saulipurhonen
Copy link
Contributor Author

I can see that the issue with Sample form has been fixed.
However for the other forms: Analysis and Run, the forms still miss some nested fields when editing.

For example: in Analysis:

  1. Select Reference Alignment for Analysis Type
  2. Select Standard for Reference assembly details
  3. Fill in the field Accession Id of Standard
  4. Click Save as Draft
  5. Edit this draft
  6. The fields append to Standard is missing

Good catch. For some reason I missed this one and the one in the Run. Sample and Experiment should work but I need to look into why it doesn't append fields in all cases.

@saulipurhonen saulipurhonen marked this pull request as draft May 28, 2021 12:42
@saulipurhonen saulipurhonen force-pushed the bugfix/edit-does-not-work-for-all-fields branch from 3c0969a to 84a3dcb Compare June 4, 2021 10:42
@saulipurhonen
Copy link
Contributor Author

All other fields except deeply nested fieldArray fields should work.

Experiment form as an example:

  1. Add new item to Expected Base Call Table and fill elevated field array.
  2. Save draft and click edit from Choose from drafts.

Added item in Expected Base Call Table isn't rendered but data is there if you look into currentObject in state.

In React Hook Form documentation for useFieldArray it seems that to get data from right registered field, we need to use whole path instead of last path element.

Path to element in this case would be: design.spotDescriptor.readSpec.expectedBaseCallTable
Instead we're trying to use only expectedBaseCallTable for element lookup and this doesn't match.

#322 fixed issue when user tries to edit form if deeply nested values exist. This might however cause issues when we'd like to render those deeply nested values. Fix passes only the last item of path to useFieldArray and I'm thinking this could be the culprit for this issue.

@saulipurhonen saulipurhonen force-pushed the bugfix/edit-does-not-work-for-all-fields branch from 84a3dcb to bf28ce3 Compare June 4, 2021 11:00
saulipurhonen and others added 5 commits June 15, 2021 17:47
Dedicated test for form edit

E2E testing
Render form correctly when editing

Display edited fields

Populate form fields

Render form correctly when editing
@hannyle hannyle mentioned this pull request Jun 16, 2021
3 tasks
@saulipurhonen saulipurhonen force-pushed the bugfix/edit-does-not-work-for-all-fields branch from 79f8d98 to 93a889b Compare June 21, 2021 11:28
@saulipurhonen
Copy link
Contributor Author

Found another problem which is:
Experiment > Expected Base Call Table > Processing > Complex Processing > Pipeline > Pipe Section

All other fields populate values correctly except Prev Step Index.

@saulipurhonen
Copy link
Contributor Author

Found another problem which is:
Experiment > Expected Base Call Table > Processing > Complex Processing > Pipeline > Pipe Section

All other fields populate values correctly except Prev Step Index.

I'm not 100% sure but the data from saved submission looks odd to me:

{
  "processing": [
    {
      "pipeline": {
        "pipeSection": [
          {
            "prevStepIndex": "test value"
          }
        ]
      }
    }
  ]
}

And in schema prevStepIndex is an oneOf typed property and has two options: String value and Null value

But it seems there's no way to tell to which option should get the value of prevStepIndex.

@blankdots, any ideas on this? Or am I missing something?

Copy link
Contributor

@blankdots blankdots left a comment

Choose a reason for hiding this comment

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

all looks good to me, only thing i noticed is this error otherwise i would have approved.

Peek 2021-06-22 16-31

to replicate

  1. Select Single Processing first
  2. Select Complex Processing
  3. see error

@saulipurhonen
Copy link
Contributor Author

all looks good to me, only thing i noticed is this error otherwise i would have approved.

Peek 2021-06-22 16-31

to replicate

  1. Select Single Processing first
  2. Select Complex Processing
  3. see error

Found this problem yesterday and created a new issue for it: #351
I think we need to unregister and re-register oneOf field when changing options.

@blankdots
Copy link
Contributor

blankdots commented Jun 23, 2021

@saulipurhonen can #351 be done as part of this issue, or we approve this one as is and continue with another PR for that
either is fine for me.

I will approve - all LGTM and we can work on the #351 on a different PR.

@saulipurhonen
Copy link
Contributor Author

@saulipurhonen can #351 be done as part of this issue, or we approve this one as is and continue with another PR for that
either is fine for me.

I think this behavior is somewhat related to the original issue, but I think we should handle it as a separate PR to keep review easier for all.

Copy link
Contributor

@hannyle hannyle left a comment

Choose a reason for hiding this comment

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

Looks good to me :)

@blankdots blankdots merged commit ccc1ca8 into develop Jun 24, 2021
@blankdots blankdots deleted the bugfix/edit-does-not-work-for-all-fields branch June 24, 2021 05:11
@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
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants