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

Duplicate options #6768

Closed
wants to merge 3 commits into from
Closed

Duplicate options #6768

wants to merge 3 commits into from

Conversation

chrismcg14
Copy link
Contributor

Is this a bugfix, or an enhancement? Please describe.
A clear and concise description of the bug you are fixing (reference issue number), or enhancement you are implementing.

This is a bugfix. When duplicating a job option, most of the properties in the new option did not match that of the original option.

Describe the solution you've implemented
A clear and concise description of what you want to happen.

Updated the toMap() Option method to properly set the properties on the returned map. Updated the _setOptionFromParams() function to properly set the properties on the new option based off returned map from the toMap method. Added inputType property to Options class.

@chrismcg14 chrismcg14 added this to the 3.3.10 milestone Feb 4, 2021
@mergify mergify bot added the 3.3.x label Feb 4, 2021
@chrismcg14
Copy link
Contributor Author

Related PRO issue: https://github.com/rundeckpro/rundeckpro/issues/1369

@mergify mergify bot added the 3.4.x label Feb 4, 2021
Copy link
Member

@gschueler gschueler left a comment

Choose a reason for hiding this comment

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

this should avoid adding a new field to the domain class.

step 1 should be create a new test case that fails with the existing behavior, and then proceed from there for implementing the solution.

I see changes that don't seem related to the problem (updating the _validateOption method), and no test cases showing what that is meant to accomplish.

any changes to the Option or EditOptsController class should have accompanying test cases. The tests should be the way to answer the question: "what is this supposed to do?" the tests should fail with existing code, and pass with the new code, showing that the behavior was broken and is now fixed.

map.put("enforcedType", "enforced")
map.enforced = true
}
if(enforced == "regex"){
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't make sense, the enforced field is a Boolean, so can't be compared to a String

@gschueler
Copy link
Member

I think i found the true source of the bug: the _duplicateOption method passes the value of option.toMap() into the params when calling _applyOptionAction here http://github.com/rundeck/rundeck/blob/main/rundeckapp/grails-app/controllers/rundeck/controllers/EditOptsController.groovy#L1001-L1001

However, the input "params" map from (form fields sent in the http request) is different than the toMap() map. normally the requests that modify the options list parse the "params" map into an Option value, however with the Duplicate request, there is not a set of input params, so the toMap() was used instead.

@gschueler
Copy link
Member

the _applyOptionAction call can instead pass params: _getParamsFromOption(newOption).

@gschueler
Copy link
Member

different fix #6784

@gschueler gschueler closed this Feb 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants