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

Remove reset handler from ruler input element #6321

Merged
merged 6 commits into from Apr 19, 2019

Conversation

Projects
None yet
3 participants
@guerler
Copy link
Contributor

commented Jun 12, 2018

This PR attempts to revise the reset handling mechanism used in the ruler input element. The PR is not complete yet. ping @jmchilton

@guerler guerler added this to the 18.09 milestone Jun 12, 2018

@guerler guerler requested a review from jmchilton Jun 12, 2018

@dannon dannon modified the milestones: 18.09, 19.01 Sep 6, 2018

@jmchilton

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

@guerler is this just marked as WIP because it is waiting on my input? I'll review and merge it if it works for me for 19.01 if you take it out of WIP.

@jmchilton

This comment has been minimized.

Copy link
Member

commented Dec 21, 2018

Yeah - so I tested this and it doesn't work yet so I'm kicking this to 19.05. It is difficult to test since there is added validation in the rule builder modal that makes it harder to get it in an error state but I think this works:

  • Open a list with the apply rules tool.
  • Assign column A (generated from the list identifier) to be the target list identifier.
  • Delete the rule that adds column A and save.
  • Click "Execute" on the tool form.
  • The rule element goes red.
  • Now fix the rules by editing them and adding column A back using add column from metadata.
  • Click save.
  • In dev, this restores the rule element to its unerrored state. In this branch, the element is still in error.

Pushing my dev merge, but feel free to rebase it away.

@jmchilton jmchilton modified the milestones: 19.01, 19.05 Dec 21, 2018

@jmchilton
Copy link
Member

left a comment

See comment in conversation.

@guerler

This comment has been minimized.

Copy link
Contributor Author

commented Dec 22, 2018

Ah ok, thanks for the description on how to reproduce the mechanism. Thanks.

@guerler

This comment has been minimized.

Copy link
Contributor Author

commented Dec 23, 2018

@jmchilton although the states remains red, is the tool executable after having fixed the rule configuration?

@guerler guerler changed the title [WIP] Remove reset handler from ruler input element Remove reset handler from ruler input element Feb 5, 2019

@guerler guerler added status/review and removed status/WIP labels Feb 6, 2019

@jmchilton

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

Re-ran the test I described and the same result. Yes, the tool can be executed - I feel like the dynamic check that it was good after returning to the tool form was good.

@jmchilton jmchilton modified the milestones: 19.05, 19.09 Apr 16, 2019

@jmchilton jmchilton merged commit dad96fc into galaxyproject:dev Apr 19, 2019

6 checks passed

api test Build finished. 449 tests run, 1 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 195 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 285 tests run, 12 skipped, 0 failed.
Details
selenium test Build finished. 151 tests run, 3 skipped, 0 failed.
Details
toolshed test Build finished. 578 tests run, 0 skipped, 0 failed.
Details

@jmchilton jmchilton removed this from the 19.09 milestone Apr 19, 2019

@jmchilton jmchilton added this to the 19.05 milestone Apr 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.