Fixes #30143 - passing root_url without section to documentation_url

[Ron-Lavi]

documentation_url expect section to be provided
also when only using root_url

[theforeman-bot]

Issues: #30143

[tbrisker]

https://theforeman.org/documentation.html is an old url that doesn't exist anymore, it redirects to the quickstart guide.
I believe this whole condition should just be: root_url + section without checking if the section is empty (since that would just return the root_url).

[ShimShtein]

The idea was to create a default url, so the user will not be redirected to 404 page, if some pages did move. Especially for stale plugins.

[tbrisker]

but right now if the section is empty it points to a stale url, while the base works just fine. And this function doesn't work for plugins anyways?

[Ron-Lavi]

Nice catch @tbrisker !
I would be ok with removing the section,
or replacing it with another url, e.g: https://theforeman.org/manuals/`version`

[ShimShtein]

After talking to @tbrisker, it looks like leftovers of our old documentation site structure. Now we can redirect to https://theforeman.org/manuals/#{SETTINGS[:version].short}/index.html. Meaning we can simplify the method to always return root_url + section (in case the section is empty - it will fall back to root_url, which is fine.

[ShimShtein]

After talking to @tbrisker, it looks like leftovers of our old documentation site structure. Now we can redirect to https://theforeman.org/manuals/#{SETTINGS[:version].short}/index.html. Meaning we can simplify the method to always return root_url + section (in case the section is empty - it will fall back to root_url, which is fine.

[Ron-Lavi]

Thanks @ShimShtein @tbrisker !
rebased and updated with your suggestion.

[tbrisker]

Test failure related

[Ron-Lavi]

Failed on:

TypeError: no implicit conversion of nil into String
    app/controllers/links_controller.rb:34:in `+'
    app/controllers/links_controller.rb:34:in `documentation_url'

Trying to use a ternary instead, e.g:
section ? root_url + section : root_url

[tbrisker]

Failed on:

TypeError: no implicit conversion of nil into String
    app/controllers/links_controller.rb:34:in `+'
    app/controllers/links_controller.rb:34:in `documentation_url'

Trying to use a ternary instead, e.g:
section ? root_url + section : root_url

The issue is in the external_url method - it calls the function with options['section'], options, so the first argument is nil if the key doesn't exist, thus the default is never triggered.
The same issue is al true for the other methods - in ruby, the default value is only used if no parameter is passed to the function, if nil is passed than it overrides the default. I have to say I find it quite odd that each of the functions in the controller expects the parameters in a different way, rather than having a common interface (e.g. taking options and extracting needed parameters from it).

[Ron-Lavi]

@tbrisker @ShimShtein should we refactor to pass only options as argument to each case in external_url?

[ShimShtein]

I prefer not to change method signatures - they are used by the theme

[ShimShtein]

Suggested change

	section ? root_url + section : root_url
	root_url + (section || '')

[Ron-Lavi]

Thanks, updated!

[Ron-Lavi]

rebased, ready for another round :)

[Ron-Lavi]

sorry, my fix got reverted on the rebase,
now it should work - tests passed locally.

[tbrisker]

APJ, thanks @LaViro !