Fix opening incorrect tag page, opened for Ansible Playbooks navigated through Repository summary page #3721

hstastna · 2018-04-05T12:32:45Z

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1560482

Problem:
Incorrect tag page was opened for tagging Playbooks navigated through Repository summary page (of some repo under Automation > Ansible > Repositories) because params[:pressed] was not properly set ("ansible_repository_tag" instead of "embedded_configuration_script_payload_tag") and then also wrong parameter went to tag method. This was caused by wrong choosing of toolbar for the page - this was caused here: https://github.com/hstastna/manageiq-ui-classic/blob/master/app/controllers/ansible_repository_controller.rb#L14

Solution:
The right toolbar for Playbooks has to be chosen, when navigating to Playbooks through Repository summary page.

Navigating to Playbooks through Repository summary page:

Navigating to Playbooks normally (Automation > Ansible > Playbooks), see the toolbar:

Note:
Original way of choosing toolbars for Ansible Playbooks/Repos/Credentials pages can cause more problems than mentioned in the BZ (link above). This is why this was changed for all of these pages, unified. Also another cases for params[:pressed] were added to button methods in appropriate controllers.

Done:

add new method center_toolbar_filename_embedded_ansible to toolbar chooser to choose toolbar for Ansible Playbooks/Repos/Credentials properly
add condition to toolbar chooser to center_toolbar_filename_classic method for proper choosing toolbar of Ansible Playbooks/Repos/Credentials pages if @display variable is set
remove unnecessary calling toolbar method from ansible playbook controller which was the original way of choosing toolbar, implemented in Add RBAC and Tagging Support to Ansible Playbooks #3522 (as the same way was already implemented in Ansible Credentials/Repositories pages)
remove unnecessary calling toolbar method from ansible repository controller because this did not work well and add new case to button method for tagging playbooks from nested list
remove unnecessary calling toolbar method from ansible credential controller because this did not work well and add new case to button method for tagging repositories displayed in a nested list (incl. small change https://github.com/ManageIQ/manageiq-ui-classic/pull/3721/files#diff-127a850b7d7b20173ad3fba88f8a25e3R238)
spec tests for the changes

Before:

After:

hstastna · 2018-04-05T12:33:02Z

@miq-bot add_label bug

hstastna · 2018-04-06T12:41:39Z

@miq-bot remove_label wip

hstastna · 2018-04-10T10:25:46Z

@miq-bot add_label gaprindashvili/yes

hstastna · 2018-04-10T10:26:49Z

@miq-bot add_label tagging

martinpovolny · 2018-04-10T13:16:22Z

app/controllers/ansible_repository_controller.rb

@@ -49,6 +48,8 @@ def button
      end
    when "ansible_repository_tag"
      tag(self.class.model)
+    when "embedded_configuration_script_payload_tag" # playbooks from nested list
+      tag(model_string_to_constant(@display))


can you pass a constant here? (rather than trusting whatever might be in @display)

miq-bot · 2018-04-26T10:33:39Z

This pull request is not mergeable. Please rebase and repush.

hstastna · 2018-04-26T12:08:53Z

@skateman Some spec tests to revieew for you here, too! ;) 🙏

hstastna · 2018-04-26T12:13:06Z

@miq-bot add_reviewer @skateman

Remove calling toolbar method from ansible repository controller because this did not work well; add new case to button method for tagging playbooks from nested list.

Remove calling toolbar method from ansible credential controller because this did not work well; add new case to button method for tagging repositories displayed in a nested list.

Add a spec test for changes, related to tagging Ansible Repositories from a nested list, in ansible credential controller.

Add a spec test for changes, related to tagging Ansible Playbooks from a nested list, in ansible repository controller.

Add spec test for changes related to Ansible Playbooks/Repositories/Credentials pages.

Add spec tests to check rendering correct toolbars for Ansible Credentials,Repositories or Playbooks list screens.

miq-bot · 2018-05-18T12:08:56Z

Checked commits hstastna/manageiq-ui-classic@0d1ddab~...8e188db with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
8 files checked, 0 offenses detected
Everything looks fine. 👍

hstastna · 2018-05-21T09:35:16Z

@martinpovolny I've added some simple tests. Do you think that it is enough or should I add anything else? Thanks! :)

martinpovolny · 2018-05-22T12:02:33Z

@skateman : did you test this in the UI?

Thx!

skateman

hstastna · 2018-05-23T14:04:28Z

@martinpovolny Can this PR be merged? It looks like fixing that BZ is important, as another BZ for the same problem was created: https://bugzilla.redhat.com/show_bug.cgi?id=1581635 (I have marked it as a dup).

…page_playbooks_through_repo Fix opening incorrect tag page, opened for Ansible Playbooks navigated through Repository summary page (cherry picked from commit 09a453a) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1583779

simaishi · 2018-05-29T17:23:47Z

Gaprindashvili backport details:

$ git log -1
commit 8680d2f0660cb155940789c668bfbc835285e695
Author: Martin Povolny <[email protected]>
Date:   Wed May 23 17:17:19 2018 +0200

    Merge pull request #3721 from hstastna/Tagging_Ansible_Incorrect_tag_page_playbooks_through_repo
    
    Fix opening incorrect tag page, opened for Ansible Playbooks navigated through Repository summary page
    (cherry picked from commit 09a453a95bf7a68ff5f5d3d4e868cfcdfe04a118)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1583779

himdel · 2018-06-13T11:06:49Z

@martinpovolny @hstastna was this really necessary?

The whole point of that toolbar helper was so that we could specify this in the controller, and not in a maze of twisty little methods elsewhere.

I would have thought that extending the helper to take a Proc that decides would have been more in line with that goal...

(And if not, setting @center_toolbar in the action where it's supposed to be different should also work.)

martinpovolny · 2018-06-13T11:49:22Z

@himdel : Right. I did not realize that this was an opportunity to move forward in that direction.

@hstastna : can you look into that in a follow up PR, please?

hstastna · 2018-06-13T12:02:25Z

@himdel @martinpovolny Yes, of course, I can look into that in a follow up PR. Thanks!

related PR: ManageIQ#3721 Move setting toolbar filename to Ansible Playbook/Credential/Repository controllers by defining new toolbar method which takes care also about setting toolbar name while displaying nested lists from summary pages.

miq-bot added bug wip labels Apr 5, 2018

hstastna force-pushed the Tagging_Ansible_Incorrect_tag_page_playbooks_through_repo branch 9 times, most recently from 571f123 to 0412356 Compare April 6, 2018 12:40

hstastna changed the title ~~[WIP] Fix opening incorrect tag page, opened for Ansible Playbooks navigated through Repository summary page~~ Fix opening incorrect tag page, opened for Ansible Playbooks navigated through Repository summary page Apr 6, 2018

miq-bot removed the wip label Apr 6, 2018

miq-bot added the gaprindashvili/yes label Apr 10, 2018

miq-bot added the tagging label Apr 10, 2018

mzazrivec assigned martinpovolny Apr 10, 2018

martinpovolny reviewed Apr 10, 2018

View reviewed changes

hstastna force-pushed the Tagging_Ansible_Incorrect_tag_page_playbooks_through_repo branch 2 times, most recently from 5e2e831 to 138b874 Compare April 10, 2018 13:58

miq-bot added the unmergeable label Apr 26, 2018

hstastna force-pushed the Tagging_Ansible_Incorrect_tag_page_playbooks_through_repo branch 2 times, most recently from e2589cc to 8b496b4 Compare April 26, 2018 11:59

miq-bot requested a review from skateman April 26, 2018 12:13

Hilda Stastna added 6 commits May 18, 2018 14:03

Remove original way of choosing toolbar for Ansible Repositories

3c7f204

Remove calling toolbar method from ansible repository controller because this did not work well; add new case to button method for tagging playbooks from nested list.

Remove original way of choosing toolbar for Ansible Credentials

348ee75

Remove calling toolbar method from ansible credential controller because this did not work well; add new case to button method for tagging repositories displayed in a nested list.

Add a spec test for tagging Repostories from a nested list

94a4b1f

Add a spec test for changes, related to tagging Ansible Repositories from a nested list, in ansible credential controller.

Add a spec test for tagging Playbooks from a nested list

b638cbd

Add a spec test for changes, related to tagging Ansible Playbooks from a nested list, in ansible repository controller.

Add spec test for changes in center_toolbar_filename_classic method

205218c

Add spec test for changes related to Ansible Playbooks/Repositories/Credentials pages.

Add spec tests to check toolbars for Ansible Credentials/Repos/Playbooks

8e188db

Add spec tests to check rendering correct toolbars for Ansible Credentials,Repositories or Playbooks list screens.

hstastna force-pushed the Tagging_Ansible_Incorrect_tag_page_playbooks_through_repo branch from 09ade7d to 8e188db Compare May 18, 2018 12:04

skateman approved these changes May 23, 2018

View reviewed changes

martinpovolny merged commit 09a453a into ManageIQ:master May 23, 2018

martinpovolny added this to the Sprint 87 Ending Jun 4, 2018 milestone May 23, 2018

simaishi added the blocker label May 29, 2018

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels May 29, 2018

Glutexo mentioned this pull request Jun 11, 2018

Add Ansible Repository refresh output page #3762

Merged

2 tasks

Glutexo mentioned this pull request Jun 19, 2018

Ansible repository refresh button on subpage #4160

Closed

hstastna mentioned this pull request Jun 20, 2018

Move setting toolbar filename to Ansible Playbook/Repository/Credential controllers #4173

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix opening incorrect tag page, opened for Ansible Playbooks navigated through Repository summary page #3721

Fix opening incorrect tag page, opened for Ansible Playbooks navigated through Repository summary page #3721

hstastna commented Apr 5, 2018 •

edited

Loading

hstastna commented Apr 5, 2018

hstastna commented Apr 6, 2018

hstastna commented Apr 10, 2018

hstastna commented Apr 10, 2018

martinpovolny Apr 10, 2018

miq-bot commented Apr 26, 2018

hstastna commented Apr 26, 2018

hstastna commented Apr 26, 2018

miq-bot commented May 18, 2018

hstastna commented May 21, 2018

martinpovolny commented May 22, 2018

skateman left a comment

hstastna commented May 23, 2018

simaishi commented May 29, 2018

himdel commented Jun 13, 2018 •

edited

Loading

martinpovolny commented Jun 13, 2018

hstastna commented Jun 13, 2018

Fix opening incorrect tag page, opened for Ansible Playbooks navigated through Repository summary page #3721

Fix opening incorrect tag page, opened for Ansible Playbooks navigated through Repository summary page #3721

Conversation

hstastna commented Apr 5, 2018 • edited Loading

hstastna commented Apr 5, 2018

hstastna commented Apr 6, 2018

hstastna commented Apr 10, 2018

hstastna commented Apr 10, 2018

martinpovolny Apr 10, 2018

Choose a reason for hiding this comment

miq-bot commented Apr 26, 2018

hstastna commented Apr 26, 2018

hstastna commented Apr 26, 2018

miq-bot commented May 18, 2018

hstastna commented May 21, 2018

martinpovolny commented May 22, 2018

skateman left a comment

Choose a reason for hiding this comment

hstastna commented May 23, 2018

simaishi commented May 29, 2018

himdel commented Jun 13, 2018 • edited Loading

martinpovolny commented Jun 13, 2018

hstastna commented Jun 13, 2018

hstastna commented Apr 5, 2018 •

edited

Loading

himdel commented Jun 13, 2018 •

edited

Loading