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

ServiceOrder deep copy #12945

Merged
merged 1 commit into from
Dec 6, 2016
Merged

ServiceOrder deep copy #12945

merged 1 commit into from
Dec 6, 2016

Conversation

jntullo
Copy link

@jntullo jntullo commented Dec 1, 2016

The SUI will be adding the ability to re-order / duplicate an order (see here and here). As a result, we need a way to deep copy a service order to include its requests.

It only copies the necessary attributes of an miq_request so that it resets attributes such as approval_state and request_state to the defaults.

@miq-bot add_label enhancement, services
@miq-bot assign @gmcculloug
cc: @syncrou

# Should be put back into the Cart state
new_service_order.state = STATE_CART
new_service_order.miq_requests = miq_requests.collect do |request|
request.class.send(:create, request.attributes.except("id", "name", "approval_state", "request_state",
Copy link
Contributor

Choose a reason for hiding this comment

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

@jntullo - Possibly these attributes should be moved to a constant.

"placed_on"))
end
new_attributes.each do |attr, value|
new_service_order.send("#{attr}=", value)
Copy link
Contributor

Choose a reason for hiding this comment

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

This worries me a bit - maybe because we're implicitly trusting what is coming in to the method new_attributes = {}. I understand the save! would validate and throw any exception. Maybe I'm being a bit too cautious.

@gmcculloug do you have any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'd feel better if we validated that the new_attributes hash included only attributes specific to the new_service_order and then passed them to the send method? - Again open to any other thoughts on this.

move request attributes to a constant

validating the attributes
@jntullo jntullo mentioned this pull request Dec 1, 2016
@gmcculloug
Copy link
Member

@tinaafitz Please review

@gmcculloug
Copy link
Member

@jntullo euwe/yes or euwe/no?

@jntullo
Copy link
Author

jntullo commented Dec 5, 2016

@gmcculloug no
@miq-bot add_label euwe/no

@miq-bot miq-bot added the euwe/no label Dec 5, 2016
@tinaafitz
Copy link
Member

@gmcculloug @jntullo Looks good.

@miq-bot
Copy link
Member

miq-bot commented Dec 6, 2016

Checked commit jntullo@3dd226a with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
2 files checked, 0 offenses detected
Everything looks good. 👍

@gmcculloug gmcculloug merged commit 5a93f72 into ManageIQ:master Dec 6, 2016
@gmcculloug gmcculloug added this to the Sprint 51 Ending Jan 2, 2017 milestone Dec 6, 2016
@jntullo jntullo deleted the order_copy branch April 11, 2017 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants