Add Ansible Repository refresh output page

[Glutexo]

Create a custom display mode for Ansible Repository show page with last refresh output capture. If an Ansible Tower Repository has this capture from its last failed refresh, the Status row becomes a link to this subpage.

Presenting this information is a workaround for cloning repositories from HTTPS servers with untrusted SSL certificates. Tower API does not offer an option to disable SSL check, so with the stdout user can at least see, why the cloning actually failed.

Along with RSpec examples for this feature, refactor a bit other tests related to the page refresh button:

• Increase test coverage of the AJAX content replacement used
• Test Playbooks nested list subpage
• Rename examples with rather misleading titles

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

Prior merging this PR, following PRs are required to be merged:

• Add configuration_script_sources.last_update_error manageiq#17290 model change
• Get stdout on ansible repository refresh manageiq-providers-ansible_tower#72 not necessarily required, but this PR doesn’t make sense without it

/cc @ZitaNemeckova, @hstastna for review

[hstastna]

@Glutexo Could you please add some screenshots if there is some change in the UI? I'm not sure if I understand your change. Thanks!

[hstastna]

And in specs, I would better use

is_expected.to have_http_status(200)

instead of is_expected.to have_http_status 200
but that's just a detail.

[mzazrivec]

Missing gettext.

[Glutexo]

@hstastna

Here there the screenshots requested. They show the Repository show page and its new output subpage. Before this PR, all Repositories looked the same. This adds a subpage (display mode) for the failed ones. The Status: failed row turns into a link.

For the non-failed repo’s too, the Status row is not clickable, she subpage can be accessed directly by the url though. The Refresh button in the toolbar should reload the subpage contents via AJAX, not returning to the Summary. The back-arrow button should return to the Summary.

• successful – summary
• successful – output
• failed – summary
• failed – output

[Glutexo]

I fixed the missing gettext spotted by @mzazrivec, squashed it and rebased onto current upstream master.

@hstastna As for the parentheses in the have_http_status method call: I agree with you, however I mimicked the style of the calls that is already there. So I leave it as it is, re-styling can be done in bulk if necessary.

Technically, the expect(…).to is a method call too, so it should be expect(response).to(have_http_status(200))…

[hstastna]

I see that you set controller.params[:display] in more it blocks exactly like this. You can use context blocks and set params[:display] in them by using let for lazy evaluation and it blocks then. For example like this:

context 'calling show_output method' do
  let(:display) { 'output' }

   it 'calls show_output method' do
     receives_init_show
     expect(controller).to receive(:show_output)
     renders
     controller.send(:button)
  end
end

If you do so, you may also want to change line 125 https://github.com/ManageIQ/manageiq-ui-classic/pull/3762/files#diff-62ca34af6656484e37abce1fb2fe1461R125 like this, to set params[:display] for the spec properly:

      let(:params) { {:action => "button", :pressed => "ansible_repository_reload", :display => display} }

[Glutexo]

Cool! I didn’t know that more nested lets can be used from outside context too. Thanks!

[Glutexo]

In reaction to a review comment by @hstastna, I grouped the “refreshing an actual repository page” specs into contexts based on the display param they use. Squashed and rebased onto the current master.

Just a note: We decided to store the output capture for all repositories, not only for the failed ones. This requires no code change in the UI, it is just not a bug when the “Status: successful” row becomes clickable. Capturing stdout makes sense for non-failed repositories as well, since there might be warnings or other useful information.

[miq-bot]

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

[Glutexo]

When I created this PR, there was a refresh button on subpages (custom display modes) as it was on main show page. When I now want to test it after rebasing, I don’t see the refresh button anymore.

Main show toolbar:

Custom display mode (subpage) toolbar:

Is it possible that toolbar behavior changed? I added a custom display mode in this PR and along with that a piece of code handling the refresh button. If the button is really no longer there, I can remove this newly introduced code and thus reduce complexity. Also in such case the refresh button handling could be simplified, not in this PR though.

Maybe @mzazrivec or @himdel could know?

[Glutexo]

@miq-bot add_label pending core

[himdel]

how is display_nested_list relevant here? Are you introducing any nested lists?

(or is this just reimplementing the parts of show that were already needed?)

[Glutexo]

@himdel This is a piece taken from the show method in a not very DRY way.

The show method doesn’t reach the custom_display_call at all, so at least this one would have to remain here. Because of that and some other struggles I decided to leave only what’s necessary here from the show method.

That is: init_show and those two ifs. The first is for playbooks subpage, which is a nested list, the second is for the new output subpage. I am however not sure whether it decreases complexity – the whole show method is not called here – or increases it – there are two decision points introduced.

However, since the Refresh button disappeared, this might not be actually needed at all. Moreover, the existing code there could be safely removed.

[himdel]

Cc @skateman , I think you may have some comments here..

(Methods in a spec.)

[skateman]

@Glutexo methods in a spec are evil, can you rewrite them using shared examples and shared contexts?

[Glutexo]

@himdel @skateman No problem, I’ll rewrite it. Would you please provide me with a link to some resource explaining the evilness of methods in specs?

[skateman]

TL;DR: mixing two patterns that do the same thing (rspec ruby vs pure ruby) is never a good idea
If you want a longer explanation, I can try next week if that's okay for you 😉

[himdel]

@Glutexo you may need to explicitly set @center_toolbar = 'ansible_repository_center', so that the right toolbar gets picked up for the new screen.

[Glutexo]

I bisected the state before rebase and after that. The refresh button disappeared with the commit 3c7f204 by @hstastna. From my first glimpse at the PR that contains it (#3721) it looks like the refresh button should not have been there in the first place. I’ll investigate it a bit more.

[Glutexo]

I get it now. The playbooks subpage used the same toolbar as its parent repository page. Because of that the Policy › Add Tags button added tags to the repository and not to the playbooks. That was addressed and fixed by #3721.

However at the same time the repository toolbar contained a refresh button. This button appeared on the playbooks subpage too. There is a piece of code that handles the button press. Thanks to the magic in the generic show mixin it works for the playbooks subpage correctly too.

Still there is 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 too great.

For a mix of reasons, this piece of code didn’t work automatically for the new stdout subpage I introduced. Because of that I made some controversial modifications to the refresh button handling. But since the button is no longer there, although not necessary intentionally, this piece of code is not necessary and can be removed from this PR.

[Glutexo]

So, the refresh button is gone for subpages (be in intentional or not). Thanks to that I could simplify this PR. Removed changes to the refresh button handling making it work for the new subpage as well as the related tests. Those can be added in case the refresh button would return.

Still this is not mergeable, because of dependencies on other PRs. It is however hopefully at least simple enough to be reviewed.

cc @himdel @hstastna

[hstastna]

@Glutexo It looks good (except the specs). Maybe you could divide the code into more commits to make your changes more clear to anyone. At least 2 commits: commit for the changes + commit for specs ;)
Question:
"If an Ansible Tower Repository has this capture from its last failed refresh, the Status row becomes a link to this subpage." - how can I achieve this (failed refresh) to test Refresh output page in the UI? Thank you!

[hstastna]

@Glutexo Travis is failing here and it looks like it is related to your changes. Specs are failing for me locally, too, with your changes (they pass without your changes). Could you check it, please? Thank you! 👍

[Glutexo]

Okay, I’ll check more deeply.

[Glutexo]

@hstastna

Failing specs

Oh, of course the specs are failing! It’s because these changes depend on a model/schema update: Please checkout https://github.com/Glutexo/manageiq/tree/add_configuration_script_source_last_update_stdout and https://github.com/Glutexo/manageiq-schema/tree/add_configuration_script_source_last_update_stdout to update the database schema and the Rails model.

Unfortunately I am currently not able to quickly test it locally, because Bundler and updates fail for me for some unknown reason. But that above is necessary to test both by rspec and manually.

How to test the UI behavior

Prerequisities

• You have to have some Ansible Repositories in your database. I assume you already have those.
• You have to have your database schema updated, so you have the last_update_stdout column in the configuration_script_source table.

Steps

• Fill in something into the last_update_stdout column of some Ansible repository.
• Find this repository in the list (Automate › Ansible › Repositories) and click it to open its show page.
• See the Status row containing success/failed and check it is a link.
• Click the row to open a separate page with the output.
• Check that there is the column contents displayed.
• Set the column value to NULL.
• Refresh the page.
• Check that no error (500) is raised and there is a friendly notice on the page.
• Go back to the main repository show page and refresh it.
• See the Status row again and check it is not a link.

[hstastna]

Of course I've updated the db schema. And specs are still failing.

[Glutexo]

@hstastna I know you said you updated the schema, but still I’d like to verify you don’t have the same problem as I did.

I double- (triple-) checked and got failing specs too. This was really because of the missing column, at least on my machine, in the vmdb_test database. After a few moments of despair I realized that I have to overload the manageiq-schema gem in my custom bundler.d Gemfile. This made the Rake task to pick my branch with the additional migration. When I fixed this and ran rake test:vmdb:setup the tests magically turned green.

[hstastna]

@Glutexo Thank you very much for your detailed explanation. Yes, sometimes we can forget about some specific things which we should do.. But.. I did the same and it didn't help 😅 Maybe I'm doing something wrong. Anyway, @skateman is better expert for specs ;)

[skateman]

Try from the manageiq core repo:

RAILS_ENV=test bin/rake db:migrate

Might help, but just a guess 😉

[Glutexo]

@hstastna Just to be sure. When you run the ansible_repository_controller.spec and it fails, take a look into test.log please. What does that say? Just to be sure it‘s not method_missing like this:

[----] F, [2018-06-20T14:49:47.574607 #50762:3fd99e43f7bc] FATAL -- : Error caught: [ActionView::Template::Error] undefined method `last_update_stdout' for #<ManageIQ::Providers::EmbeddedAnsible::AutomationManager::ConfigurationScriptSource:0x00007fb347c30418>
Did you mean?  last_updated_on

which would mean that the schema is bad. If it’s something different, then I am wrong with all those schema-rants for the whole time.

[Glutexo]

@Fryguy requested a different name for the new column. I changed it in the schema and in the model as well as here.

[Glutexo]

@hstastna I’d prefer not to split the commit to have executive code in a different one than tests. I like that the commit is atomic and tested by itself. Moreover, the spec changes are so little that it doesn't make much sense to extract those. Sorry. 😔

May I ask you to check running the specs with the new column name?

[miq-bot]

Checked commit Glutexo@c847860 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🍰

[hstastna]

@Glutexo Finally, the specs passed 👍 for me, locally.

[tumido]

All related PRs merged now.

@miq-bot remove_label pending core

[himdel]

Tested, looks good 👍 :)

The only thing:

Fill in something into the last_update_stdout column of some Ansible repository.

this is called last_update_error now.