-
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
Fix missing flash_msg_div in ops/settings/zone_form #4583
Conversation
I really broke this usecase. But your fix in not a perfect one ;-) When I did the breaking PR #4405 I really fixed the DOM id problem in Editing. You fix fixes the missing DOM id in Adding and reintroduces the problem in Editing. The root clause is that the same partial is used in 2 different contexts. In one of the context there already is a flash_message, in the other one there is not.
Please, try to figure a better fix. One that is correct in both usecases. Hint: If you cannot find a nice way, you can add one more partial with the flash message that would include the partial w/o the message and render different partials for add/edit. You can also add a variable to drive that. Maybe there's a better solution, some reordering of the partials and/or divs. |
app/views/ops/_zone_form.html.haml
Outdated
@@ -1,5 +1,7 @@ | |||
- url = url_for_only_path(:action => 'zone_field_changed', :id => (@zone.id || "new")) | |||
- disabled_name = [email protected]? && !@edit[:current][:name].nil? |
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.
Would it make more sense to rename the variable to "editing"?
And what about setting the variable in the controller?
Also the condition seems a bit as if the empty name was an effect of editing so determining if editing is happening by this does not look very readable. Can you simplify the condition if you move it to the controller?
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.
What do you think now?
I changed the variable name to "editing" and moved the condition to the controller, where I set editing to true in editing branch of settings_replace_right_cell.
385550e
to
25d6b61
Compare
Travis failure seems related. |
Checked commit rvsia@b3de866 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
@martinpovolny It should be OK now. |
https://bugzilla.redhat.com/show_bug.cgi?id=1623912
Description of problem:
No error message when creating duplicate zone in the same region
Steps to Reproduce:
Create a domain and a namespace
Create a class (1) within the namespace that has both name and display name
Create an another class (2) within the same namespace, this time only with name and NO display name
Copy the class only with name (2) within the same domain and namespace, just give it a new name (3), safest way to do it right is to lock all domains except the one you work with right now.
There's no error message when trying to add zone with empty name.
Should be "Name can't be blank".
There's no error message when trying to add zone with empty description.
Should be "Description is required".
Before
After
Caused by #4405
@miq-bot add_label gaprindashvili/no, bug