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

Fixes issue #65 #66

Merged
merged 5 commits into from
Feb 22, 2022
Merged

Fixes issue #65 #66

merged 5 commits into from
Feb 22, 2022

Conversation

reynoldtan
Copy link

@reynoldtan reynoldtan commented Jan 27, 2022

This PR fixes Issue #65 - Unable to modify instance.

The error is caused by hook_validate when modifying an instance where there is another instance and both share the same organism. This fix takes into account sequence assembly information before checking for identical instance.

To test:
// No two instances share same organism and empty sequence assembly

  1. Create an instance and set organism to X and sequence assembly field to empty.
  2. Create a similar instance to step 1, except set organism different to X so that when you modify this (step 2 instance) and set organism to match X (step 1 instance) it should trigger the validation.

// No two instances share same organism and same sequence assembly

  1. Find an instance and note organism and sequence assembly.
  2. Create a new instance, set the organism other than the organism in step 1 (as noted) and sequence assembly to be the same in step 1.
  3. Modify instance in step 2 and change the organism to match the organism in step 1. This should trigger the validation, an existing instance matched the organism and sequence assembly information.

Knowing what triggers validation, test fields organism, description, start location and tracks to display as well.

@dsenalik
Copy link

dsenalik commented Jan 28, 2022

There is a pull request that is a fix for your tests failing, see

tripal/tripal#1237

tripal/tripal#1249

Copy link
Member

@laceysanderson laceysanderson left a comment

Choose a reason for hiding this comment

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

✅ Code looks good

The validation is good too and testing worked as expected :-)

Only one thing to fix. Right now it disables the analysis form field on edit even it is not yet. Admin should be able to go back and add analysis after the initial creation. This is common if the first time they didn't bother because it was the only assembly or the functionality didn't exist yet, but now there are many assemblies and they want to make it more clear.

@reynoldtan
Copy link
Author

The latest commit fixes the issue of field sequence assembly set to disabled by default, and as part of the fix, it will now save value set to this field when saving modifications.

Please note that with this fix, the edit form will only allow modification of sequence assembly when it was not set previously in create/add form.

To test:

  1. Create an instance and leave sequence assembly field empty.
  2. Modify this instance, and set a value for sequence assembly field.
  3. Click save. Validation rules in the previous commit still apply.

Copy link
Member

@laceysanderson laceysanderson 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 and works perfectly -Thank you @reynoldtan

@laceysanderson laceysanderson merged commit 8d5f716 into 7.x-3.x Feb 22, 2022
@laceysanderson laceysanderson deleted the unable-to-modify branch February 22, 2022 22:03
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.

3 participants