Fixes #24675 - Update host name for Manifest refresh

[johnsonm325]

Comments in redmine mention the need to update the hard-coded link in the file so it points to
https://subscription.rhsm.redhat.com/subscription/consumers/. This fixes the issue with updating
while the server is pointed to a proxy, but only covers up the root of the issue. The root of the issue
is that if an old manifest is refreshed, then the old host name will be accessed. The true fix is to
update the documentation to add https://subscription.rhn.redhat.com/subscription/consumers/ to the
whitelist hostnames.

This bug fix updates the file so that if no apiUrl is present, then it will default to the updated
host.

[theforeman-bot]

Issues: #24675

[theforeman-bot]

There were the following issues with the commit message:

• commit message for ceba4f1 is not wrapped at 72nd column
• commit message for ceba4f1 is not wrapped at 72nd column
• commit message for ceba4f1 is not wrapped at 72nd column
• commit message for ceba4f1 is not wrapped at 72nd column

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

---

This message was auto-generated by Foreman's prprocessor

[theforeman-bot]

There were the following issues with the commit message:

• commit message for 21384e0 is not wrapped at 72nd column
• commit message for 21384e0 is not wrapped at 72nd column
• commit message for 21384e0 is not wrapped at 72nd column
• commit message for 21384e0 is not wrapped at 72nd column
• commit message for 21384e0 is not wrapped at 72nd column

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

---

This message was auto-generated by Foreman's prprocessor

[jturel]

Expanding test coverage where missing is a great thing! The tests you've written articulate the expected behavior nicely.

Since this is a new file and a small example I'd like to talk about how I would have written this test since it makes for a good illustration.

                                                                                                                                                  
module Katello                                                                                                                                    
  class ProviderTest < ActiveSupport::TestCase                                                                                                    
    class OwnerUpstreamUpdateTest < ActiveSupport::TestCase                                                                                       
      let(:provider) { katello_providers(:redhat) }                                                                                               
      let(:api_url) { 'http://theforeman.org' }                                                                                                   
      let(:expected_url) { api_url }                                                                                                              
                                                                                                                                                  
      def setup                                                                                                                                   
        @upstream_params = { 'apiUrl' => api_url, 'idCert' => { 'key' => '', 'cert' => ''}}                                                       
        @expected_params = [expected_url, "", "", nil, { capabilities: [], facts: { distributor_version: 'sat-6.3' } }]                           
        Resources::Candlepin::UpstreamConsumer.expects(:update).with(*@expected_params)                                                           
        Resources::Candlepin::CandlepinPing.stubs(:ping).returns('managerCapabilities' => [])                                                     
      end                                                                                                                                         
                                                                                                                                                  
      it 'calls the apiUrl in the manifest' do                                                                                                              
        provider.owner_upstream_update(@upstream_params, {})                                                                                      
      end                                                                                                                                         
                                                                                                                                                  
      context 'apiUrl missing from the manifest' do                                                                                               
        let(:api_url) { nil }                                                                                                                     
        let(:expected_url) { 'https://subscription.rhsm.redhat.com/subscription/consumers/' }                                                     
                                                                                                                                                  
        it 'falls back to the default' do                                                                                                         
          provider.owner_upstream_update(@upstream_params, {})                                                                                    
        end                                                                                                                                       
      end                                                                                                                                         
    end                                                                                                                                           
  end                                                                                                                                             
end

I observed one thing about your setup method: it is very specific to testing owner_upstream_update. I also noted that the provider code has a significant number of untested lines. With that in mind I envisioned that one day there would be many more tests in this file.

The reimagined approach makes it easier to add tests in the future and some of that is achieved by isolating the owner_upstream_update tests in to it's own subclass (OwnerUpstreamUpdateTest). It'll prevent the setup method from growing ever-larger and spanning many tests.

I also use some different syntax such as let, context, and it. This makes my approach more of a spec test than a unit test. Not only does it serve to document the behavior in plain language, but things like let allow you to override certain bits. Notice how the two tests don't need to manipulate @expected_params and @upstream_params. To me it makes the tests way more easy to interpret and perhaps more importantly - to modify them later.

The last thing I did, a bit of a trick, is use the splat operator to expand the contents of @expected_params into the with method. http://blog.honeybadger.io/ruby-splat-array-manipulation-destructuring/

Anyway, I just wanted to illustrate another approach and you'll see this format elsewhere in our tests. At a minimum I'd like you to put your test methods into a subclass like OwnerUpstreamUpdateTest so that your setup method is more isolated to the needs of your test since future authors will appreciate it.

[johnsonm325]

Thanks! Yeah, I'll use the entire thing because I think it's much better than what I've done.

[jturel]

couple comments in here that aren't needed

[jturel]

Instead of duplicating the url what about

@upstream_params['apiUrl'] = @expected_params[:url]

Nitpick: how about a shorter url? http://theforeman.org for example

[jturel]

What do you think about storing this URL in a constant that way it won't be duplicateD?

[jturel]

This looks very nice. Just need to rename your commit "Fixes #24675 - TEST"

[johnsonm325]

Done.

[jturel]

[test katello]

[jturel]

Nice! ACK