Ansible repository refresh button on subpage

[Glutexo]

Changes in #3721 by @hstastna made refresh button on playbooks subpage disappear. It is now only present on the main repository show page. Was this an intention?

There is a [piece of code] in ansible_repository_controller.rb handling the refresh button. It contains some rather obscure manipulation with the @display and params[:display] values:

manageiq-ui-classic/app/controllers/ansible_repository_controller.rb

Line 44 in 37a9337

params[:display] = @display if @display

followed directly by

manageiq-ui-classic/app/controllers/mixins/generic_show_mixin.rb

Line 121 in 37a9337

@display = params[:display] || default_display

. This still doesn’t work well if multiple browser windows are used.

If we want to have the refresh button there, I’d suggest:

• Simplify (or at least comment) the @display and params[:display] handling. Currently it is not easily understandable what is going on there.
• Fix the handling so it works even if I switch a display mode in another window. This can be achieved by passing the display mode in the button press request and it could solve the previous point too.
• Make the refresh work not only for nested list, but for custom display lists too. There are not any at the moment, but they are planned. This can be achieved by using a following snippet in the handling method branch instead of calling GenericShowMixin#show.

      init_show

      template = 'show'
      if self.class.display_methods.include?(params[:display])
        display_nested_list(params[:display])
      elsif self.class.custom_display_modes.include?(params[:display])
        custom_display_call(params[:display])
        template = custom_display_method_name(params[:display])
      end

In case the refresh button should not be present on the subpages, it would be good to remove the code that is there only to support this feature:

• Spec example for the Playbooks subpage.

  manageiq-ui-classic/spec/controllers/ansible_repository_controller_spec.rb

  Line 54 in 09a453a

  it 'reload button will stay on the nested list' do

  .
• Probably this line

  manageiq-ui-classic/app/controllers/ansible_repository_controller.rb

  Line 43 in 09a453a

  params[:display] = @display if @display

  .

[hstastna]

@Glutexo In #3721 I removed only the lines of the code which I have added before by mistake, while adding tagging feature for Ansible Playbooks, ... (for example here: #3522) It turned out that that PR caused many problems (I mean the way I've added toolbar to the pages), especially for nested lists, tagging, etc. I fixed that by #3721. While displaying nested lists, Refresh button was never designed to be there. If you need Refresh button for some reason in Ansible Playbooks, ..., you need to make changes in app/helpers/application_helper/toolbar/ansible_playbooks_center.rb.

[Glutexo]

@hstastna I don’t need that button. It might make sense to have one there, since it is on the main show page, but if it was not intended, it’s still fine not to have it. My only concern was that there is already some code that handles this button on the Playbooks subpage.

I can prepare a PR that removes this remnants. 🧹 That will clean up the code (and specs) a bit and also gets rid of the @display/params[:display] juggling. Which I think is 👍 .

[hstastna]

@Glutexo Where exactly is the code that "handles this button on the Playbooks subpage" ? I can't see it, at least not in ansible playbook controller where I would expect that.

[Glutexo]

It‘s this line:

manageiq-ui-classic/app/controllers/ansible_repository_controller.rb

Line 43 in 2985663

params[:display] = @display if @display

It tries to preserve the @display value (from memory I guess) by pretending it came in a request. It is then picked up by GenericShowMixin#init_show and stored back into @display instead of its default value 'main'.

If it wasn’t there, pushing the refresh button on the playbooks subpage would cause the main div content to be replaced by the main show content. It still doesn’t work well however, because @display is not stateless and must contain the correct value. If you move to main repository show page (e. g. in a different window) and then press the button on this page, it will still load the main page and not the playbooks subpage.

[hstastna]

@Glutexo It's because Ansible Repositories have Refresh button. Others not. But.. I still don't understand the problem well. Could you write down the exact steps to reproduce the problem? And let's assume that we don't want Refresh button on any subpage.

[Glutexo]

@hstastna If we assume that we don’t need the refresh button on the subpages, then it’s not necessary to fix this line and reproduce the bug it causes. In such case, the line is only redundant and can be removed with a few spec examples.

I created a PR that removes this unused code: #4163

If you were still interested in why this line was troublesome in the first place, here are steps to reproduce:

1. Open an Ansible Repository show page.
2. Open Playbooks of that repository using a link on that show page.
3. In another browser window, open again the the repository main show page.
4. In the original browser window with the Playbooks subpage click the Refresh button. (Which was present in earlier revisions, but is not now.)

If you do this, the contents of the main div is replaced by the repository’s main show page. Normally it should be replaced by an updated version of the playbooks subpage contents. This doesn’t matter anymore though if the code is going to be removed and there is no intention in returning the reload button to the subpage.

[hstastna]

Thank you, @Glutexo, for this great explanation. Yes, and I also have found the PR which added params[:display] = @display if @display to the code (#2419) and this made things more clear to me, too :)