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.

Sign up for GitHub

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

Jump to bottom

Allow error messages in ServiceTemplate.validate_order #19186

Merged
merged 8 commits into from
Aug 27, 2019
Merged

Allow error messages in ServiceTemplate.validate_order #19186

merged 8 commits into from
Aug 27, 2019

Conversation

ghost
Copy link

@ghost ghost commented Aug 21, 2019 •
edited by kbrock
Loading

For V2V, we want to return a meaningful message to the user when the service isn't orderable.
In current implementation, the ServiceTemplate.validate_order method only returns a boolean.

This PR makes it return explicit error messages. The error messages are aggregated with the workflow.validate_dialog error messages, so the user has more information why the service template order has failed.

This requires a follow-up PR in ManageIQ/manageiq-api, because it expects a boolean, and also to add the error messages to the response.

RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1744197
Depends on #19203
see also ManageIQ/manageiq-api#656

@ghost ghost mentioned this pull request Aug 21, 2019
Merged
@ghost
Copy link
Author

ghost commented Aug 21, 2019

@miq-bot add-label enhancement, ivanchuk/yes, wip

@miq-bot miq-bot changed the title Allow error messages in ServiceTemplate.validate_order [WIP] Allow error messages in ServiceTemplate.validate_order Aug 21, 2019
@ghost
Copy link
Author

ghost commented Aug 24, 2019

@miq-bot remove-label wip
@miq-bot add-reviewer @d-m-u
@miq-bot add-reviewer @agrare

@miq-bot miq-bot changed the title [WIP] Allow error messages in ServiceTemplate.validate_order Allow error messages in ServiceTemplate.validate_order Aug 24, 2019
@miq-bot miq-bot removed the wip label Aug 24, 2019
@miq-bot miq-bot requested review from d-m-u and agrare August 24, 2019 07:09
@djberg96
Copy link
Contributor

Seems reasonable to me, though I'm wondering why this isn't a bonafide validation on the model somewhere.

@d-m-u
Copy link
Contributor

d-m-u commented Aug 26, 2019

Some of the template errors get stored in template_valid_error_message, do they matter for this?

@ghost
Copy link
Author

ghost commented Aug 26, 2019

@d-m-u, yes they could be added. That's another way of validating something, with a different output type. Maybe in another PR ?

@agrare
Copy link
Member

agrare commented Aug 26, 2019

@fdupont-redhat this looks like it might be better to have a supports_order? on ServiceTemplate with supports_feature_mixin

@djberg96 djberg96 mentioned this pull request Aug 26, 2019
Merged
@djberg96
Copy link
Contributor

@fdupont-redhat @agrare #19203

@ghost ghost force-pushed the v2v_allow_error_messages_in_service_template_orderable branch from 0c14431 to 3267d93 Compare August 26, 2019 20:36
agrare
agrare reviewed Aug 26, 2019
View reviewed changes
app/models/service_template.rb Outdated
@@ -353,7 +356,7 @@ def validate_template
end

def validate_order
service_template_catalog && display
supports_order?
Copy link
Member

Choose a reason for hiding this comment

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

This could probably just be another alias right?

Like

alias orderable?     supports_order?
alias validate_order supports_order?

Copy link
Author

Choose a reason for hiding this comment

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

Good point.

@agrare
Copy link
Member

agrare commented Aug 26, 2019

This looks good @fdupont-redhat just the one comment

@agrare agrare self-assigned this Aug 26, 2019
@miq-bot
Copy link
Member

miq-bot commented Aug 27, 2019

Checked commits fabiendupont/manageiq@d59a445~...e941a51 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 0 offenses detected
Everything looks fine. ⭐

agrare
agrare reviewed Aug 27, 2019
View reviewed changes
app/models/service_template_transformation_plan.rb
unsupported_reason_add(:order, 'All VMs of the migration plan have already been successfully migrated') if unmigrated_vms.blank?
end
alias orderable? supports_order?
alias validate_order supports_order?
Copy link
Member

Choose a reason for hiding this comment

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

Hm I'm surprised you needed the alias in the parent and child classes

Copy link
Member

Choose a reason for hiding this comment

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

think alias_method in the parent class would have fixed this

agrare added a commit that referenced this pull request Aug 27, 2019
@agrare
Merge pull request #19186 from fdupont-redhat/v2v_allow_error_message…
7b2880c 
…s_in_service_template_orderable

Allow error messages in ServiceTemplate.validate_order
@agrare agrare merged commit e941a51 into ManageIQ:master Aug 27, 2019
@agrare agrare added this to the Sprint 119 Ending Sep 2, 2019 milestone Aug 27, 2019
@ghost ghost deleted the v2v_allow_error_messages_in_service_template_orderable branch August 27, 2019 11:54
simaishi pushed a commit that referenced this pull request Nov 4, 2019
@agrare @simaishi
Merge pull request #19186 from fdupont-redhat/v2v_allow_error_message…
4ffb6e1 
…s_in_service_template_orderable

Allow error messages in ServiceTemplate.validate_order

(cherry picked from commit 7b2880c)

https://bugzilla.redhat.com/show_bug.cgi?id=1768520
@simaishi
Copy link
Contributor

simaishi commented Nov 4, 2019

Ivanchuk backport details:

$ git log -1
commit 4ffb6e17140861c19326b174588b1fb379a2b622
Author: Adam Grare <[email protected]>
Date:   Tue Aug 27 07:53:34 2019 -0400

    Merge pull request #19186 from fdupont-redhat/v2v_allow_error_messages_in_service_template_orderable
    
    Allow error messages in ServiceTemplate.validate_order
    
    (cherry picked from commit 7b2880c6530c906194c2b27e6a9bd1167b796382)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1768520

@miq-bot miq-bot changed the title Allow error messages in ServiceTemplate.validate_order [WIP] Allow error messages in ServiceTemplate.validate_order Feb 7, 2024
@miq-bot
Copy link
Member

miq-bot commented Feb 7, 2024

@ghost Cannot add the following reviewer because they are not recognized: d-m-u

@miq-bot miq-bot requested a review from agrare February 7, 2024 15:43
@kbrock kbrock changed the title [WIP] Allow error messages in ServiceTemplate.validate_order Allow error messages in ServiceTemplate.validate_order Feb 7, 2024
@kbrock kbrock mentioned this pull request Feb 7, 2024
Merged
23 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@d-m-u d-m-u Awaiting requested review from d-m-u

@agrare agrare Awaiting requested review from agrare agrare is a code owner

Assignees

@agrare agrare

Labels
enhancement ivanchuk/backported ivanchuk/no
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@djberg96 @d-m-u @agrare @miq-bot @simaishi @kbrock @fabiendupont