-
Notifications
You must be signed in to change notification settings - Fork 356
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
Replace Host.validate_destroy with supports feature #7639
Replace Host.validate_destroy with supports feature #7639
Conversation
9cd611d
to
912ad58
Compare
@miq-bot cross-repo-tests including ManageIQ/manageiq#21073 |
@agrare 'cross-repo-test(s)' was given invalid repo names and cannot continue
|
@miq-bot cross-repo-tests ManageIQ/manageiq#21073 |
Checked commit agrare@912ad58 with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint |
From Pull Request: ManageIQ/manageiq-ui-classic#7639
From Pull Request: ManageIQ/manageiq-ui-classic#7639
Weird spec:jest passes locally, I'll investigate |
@h-kataria I'm not able to get this to fail locally but it looks like this jest failure is also on master can you help out here? Not sure what this error means or how to fix it. |
@agrare can you try `yarn test -u˙ on the latest master and commit the changes, that should fix the CI, it should create a new snapshot. |
@h-kataria when I run that I don't see an local changes, but again this wasn't failing for me locally |
@himdel can you help with jest failures? |
@h-kataria SecondaryButtonSet is specifically a Carbon thing. See https://github.com/search?q=SecondaryButtonSet&type=code. I bet carbon updated and broke something. |
Merging since the jest failure is unrelated. |
jest fixed in #7645 :) |
Marking |
…th_supports_feature Replace Host.validate_destroy with supports feature (cherry picked from commit 54cc0a5)
Backport details
|
if !validation[:available] | ||
add_flash(validation[:message], :error) | ||
if !host.supports?(:destroy) | ||
add_flash(host.unsupported_reason(:destroy), :error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Fryguy so, if the model has N_() for the unsupported reason, then all the UI callers have to translate that via _() in places like this. Is that correct? Asking because of this discussion in a different PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I believe so. From what I can tell, N_() just tells the i18n engine to consider that string for translation (i.e. put it in the catalog), but don't actually translate it. That's why it's used for things like constants and enums. However you have to call _() to actually translate it. If you were to use _() on a constant it would translate once at constant resolution time, then never translate again for the user.
Depends:
Fixes ManageIQ/manageiq-providers-openstack#98