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

Fix missing flash_msg_div in ops/settings/zone_form #4583

Merged
merged 1 commit into from
Sep 4, 2018

Conversation

rvsia
Copy link
Contributor

@rvsia rvsia commented Sep 3, 2018

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:

  1. Create a domain and a namespace

  2. Create a class (1) within the namespace that has both name and display name

  3. Create an another class (2) within the same namespace, this time only with name and NO display name

  4. 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.

  5. There's no error message when trying to add zone with empty name.
    Should be "Name can't be blank".

  6. There's no error message when trying to add zone with empty description.
    Should be "Description is required".

Before
screenshot from 2018-09-03 11-55-44

After
screenshot from 2018-09-03 11-53-55

Caused by #4405

@miq-bot add_label gaprindashvili/no, bug

@rvsia
Copy link
Contributor Author

rvsia commented Sep 3, 2018

@martinpovolny

@martinpovolny
Copy link
Member

martinpovolny commented Sep 3, 2018

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.

  • Adding is done w/o Tabs,
  • Editting is done w/ Tabs.

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.

@@ -1,5 +1,7 @@
- url = url_for_only_path(:action => 'zone_field_changed', :id => (@zone.id || "new"))
- disabled_name = !@zone.name.nil? && !@edit[:current][:name].nil?
Copy link
Member

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?

Copy link
Contributor Author

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.

@rvsia rvsia force-pushed the BZ1623912 branch 5 times, most recently from 385550e to 25d6b61 Compare September 3, 2018 13:35
@rvsia rvsia changed the title [WIP] Fix missing flash_msg_div in ops/settings/zone_form Fix missing flash_msg_div in ops/settings/zone_form Sep 3, 2018
@miq-bot miq-bot removed the wip label Sep 3, 2018
@martinpovolny martinpovolny self-assigned this Sep 3, 2018
@martinpovolny martinpovolny added this to the Sprint 94 Ending Sep 10, 2018 milestone Sep 3, 2018
@martinpovolny
Copy link
Member

Travis failure seems related.

@martinpovolny martinpovolny removed this from the Sprint 94 Ending Sep 10, 2018 milestone Sep 3, 2018
@miq-bot
Copy link
Member

miq-bot commented Sep 4, 2018

Checked commit rvsia@b3de866 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

@rvsia
Copy link
Contributor Author

rvsia commented Sep 4, 2018

@martinpovolny It should be OK now.

@martinpovolny martinpovolny added this to the Sprint 94 Ending Sep 10, 2018 milestone Sep 4, 2018
@martinpovolny martinpovolny merged commit b992cea into ManageIQ:master Sep 4, 2018
@rvsia rvsia deleted the BZ1623912 branch September 17, 2019 12:01
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.

3 participants