[RFE] Add the /api/product_info route with product and branding info #438

skateman · 2018-07-30T11:59:58Z

If we want to customize the product name and branding, it's necessary to have a request that returns the product info without authentication. Also I would like to use this for the about modal, so adding the branding to the /api route as well.

@miq-bot add_reviewer @abellotti

RFE BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1471301

juliancheal · 2018-07-30T14:21:01Z

So we want to return something like

      res = {
        :name         => ApiConfig.base.name,
        :description  => ApiConfig.base.description,
        :version      => ManageIQ::Api::VERSION,
        :versions     => entrypoint_versions,
        :settings     => user_settings,
        :identity     => auth_identity,
        :server_info  => server_info,
        :product_info => product_info
      }

from the index method, but without needing authentication

skateman · 2018-07-30T14:34:59Z

@juliancheal do we really need all those things? Some of them aren't even available without login.

juliancheal · 2018-07-30T14:35:47Z

@skateman Well i didn't mean all the things my mistake.

abellotti · 2018-07-30T14:38:22Z

app/controllers/api/api_controller.rb

@@ -1,7 +1,7 @@
 module Api
  class ApiController < Api::BaseController
    def options
-      head(:ok)
+      render_resource :product, product_info


we need this under a product_info element, maybe just render :json => { :product_info => product_info }

Makes sense, I need 3 more items Settings.server.custom_logo, Settings.server.custom_login_logo and Settings.server.custom_brand (all boolean values). Do you have any idea how to structure them? Or is it okay to add it into product_info?

product_info is more geared toward MiQ itself, above may fit better under server_info, or maybe even a new section like brand_info.

Maybe we can make use of ManageIQ/manageiq#17755 for the version info? I'm not sure what the rest of the information is.
Why do we want to return booleans for things like Settings.server.custom_logo? (I don't know how it's used) Wouldn't it make more sense to have a logo key with a value of the href to the image or something like that?

@bdunne we want them booleans because they are already booleans, in the OPS UI there's a checkbox for using the custom image. The image itself is on a fixed location, so the filename is static for all the three cases. So historical reasons, that's why :)

@abellotti I think I'll go with the brand_info, so

{ :product_info => product_info, :brand_info => ... }

skateman · 2018-07-30T14:38:31Z

@juliancheal a "filtered" version of this would make sense, I'd definitely need some extra stuff from settings as well. The question here is if it's okay to use the OPTIONS for this? cc @abellotti @bdunne

juliancheal · 2018-07-30T14:45:56Z

@skateman yes a filtered version would be perfect. I wonder if it could just be on index. Return one set if authorised and another if not? It seems options is used more for "describe the communication options" unless I'm mistaken.

skateman · 2018-07-30T14:48:20Z

@juliancheal the authorization is a before_action and it's a lot harder to catch it than to work with the OPTIONS

bdunne · 2018-07-30T14:56:10Z

Any reason that this wouldn't be under a new /api/product_info endpoint? Also, I think we already allow GET on /api/ping without authentication.

skateman · 2018-07-30T14:58:22Z

@bdunne sounds reasonable, and what about the structure of the data?

bdunne · 2018-07-30T17:23:37Z

app/controllers/api/api_controller.rb

@@ -20,6 +20,15 @@ def index
      render_resource :entrypoint, res
    end

+    def product_info


Won't this be overridden by https://github.com/ManageIQ/manageiq-api/pull/438/files#diff-0ef0779741ad3209b46314a66c353351R89 ?

Is this also implying that we already have this endpoint?

I should not make PRs when I'm tired 😢 I'll fix this tomorrow 😕 😴

I don't think so, because the one below is private.

Yup, my bad ... fixed now.

Does this fix it though? It seems this break API compatibility because the old product_info doesn't match the new one.

Oh, you're right, is it okay if I merge the branding_info into product_info?

I'm fine with putting the branding info under product_info, but did we somehow have /product_info before?

No, it's a new thing.

Fryguy · 2018-07-30T17:46:33Z

app/controllers/api/api_controller.rb

+      {
+        :custom_brand      => Settings.server.custom_brand,
+        :custom_logo       => Settings.server.custom_logo,
+        :custom_login_logo => Settings.server.custom_login_logo


What are these settings and where do they come from? The only one in the settings template is the custom_logo, and that one is just a boolean.

@Fryguy ManageIQ/manageiq#17773 forgot to include, sorry

bdunne · 2018-07-31T13:42:36Z

app/controllers/api/api_controller.rb

      {
        :name                 => I18n.t("product.name"),
        :name_full            => I18n.t("product.name_full"),
        :copyright            => I18n.t("product.copyright"),
        :support_website      => I18n.t("product.support_website"),
-        :support_website_text => I18n.t("product.support_website_text"),


Any reason this changed?

Nope...reverting.

skateman · 2018-10-08T17:40:32Z

@bdunne @Fryguy @abellotti any other issues? Can this be merged?

bdunne · 2018-10-08T18:25:08Z

Aside from the tests failing, it looks good to me

skateman · 2018-10-09T09:33:11Z

@bdunne it seems like we cannot call ActionController::Base.helpers.image_path from the API at all because the asset load_path isn't complete. Probably something from the UI classic repo isn't loaded. I think we don't want to depend on the UI repo at all if possible, so let's try something different.

What do you think about implementing a safe-lookup for the branding images? If the lookup error happens, i.e. assets aren't available, we don't have the UI and so we don't really need the branding at all, so we just don't return the fields in the response. And if there is no lookup error, the UI is intact and we return with the correct paths for the branding images.

himdel · 2018-10-09T10:26:52Z

Calling image_path requires all application.css and application.js depencencies to exist, so.. until all external dependencies are moved off the asset pipeline, you need to call yarn before using image_path.

(Unfortunately this will still take a while, we'll need to upgrade codemirror in the process.)

So... I suggest adding rake update:yarn (no need for the whole update:ui here), either to bin/setup or to .travis.yml right after bin/setup.

skateman · 2018-10-09T11:26:10Z

Well, I don't think the API team will be happy about having yarn in their repo. But let's wait for their decision...

Fryguy · 2018-10-09T14:45:33Z

app/controllers/api/api_controller.rb

@@ -20,6 +20,15 @@ def index
      render_resource :entrypoint, res
    end

+    def product_info


Does this fix it though? It seems this break API compatibility because the old product_info doesn't match the new one.

skateman · 2018-10-10T14:46:51Z

According to @Fryguy's comment and discussing with @bdunne I updated the product_info route to return a flat hash that includes the branding_info if the UI is available, i.e. the asset_path won't fail with a Sprockets::FileNotFound exception.

bdunne · 2018-10-11T15:08:57Z

app/controllers/api/api_controller.rb

+
+    def branding_info
+      {
+        :brand      => branding_image_path(Settings.server.custom_brand, 'layout/brand.svg'),


Why pass Settings.server.custom_brand into branding_image_path if the method doesn't do anything with it?

I don't really understand this, why it would not do anything with it? The branding_image_path returns the path to the custom or default brand depending on what's available.

Does the method need the branding_ prefix? It is not branding specific. If we don't need to look up the path for the value from settings, I would expect something like :brand => Settings.server.custom_brand || image_path('layout/brand.svg')

bdunne · 2018-10-11T15:09:33Z

app/controllers/api/api_controller.rb

+      {
+        :brand      => branding_image_path(Settings.server.custom_brand, 'layout/brand.svg'),
+        :logo       => branding_image_path(Settings.server.custom_logo, 'layout/login-screen-logo.png'),
+        :login_logo => Settings.server.custom_login_logo ? Settings.server.custom_login_logo : nil


Do we need the ternary operator here?

Yes because it can also be false and that's not caught by compact

Maybe Settings.server.custom_login_logo.presence then? Sorry, I was expecting a string if it existed or nil

bdunne · 2018-10-11T15:11:28Z

app/controllers/api/api_controller.rb

+        :identity      => auth_identity,
+        :server_info   => server_info,
+        :product_info  => product_info_data,
+        :branding_info => branding_info


Do we still need this if it's also nested under product_info?

Well, we need to be able to access this without a login, so the SUI login screen can consume it.

Doesn't https://github.com/ManageIQ/manageiq-api/pull/438/files#diff-b4f92b4e361ffab4fbb91c68eb3f77bbR23 remove the requirement for being logged in?

No, the /api route isn't available without login as it contains the identity part that needs a logged in user.

Do we still need this if it's also nested under product_info?

What I meant was that we'll have this, right?

get api_entrypoint_url { "name" : "ManageIQ", "description" : "words", ..., "product_info" : { ..., "branding_info" : {...} }, "branding_info" : {...} }

Oooh, you're right 🙄 I don't know why I left that line there 😢

skateman · 2018-10-11T16:11:40Z

@bdunne just to clarify, the :product_info available from the root /api route is not accessible without a login, even though the OPTIONS is allowed. I had to expose the /api/product_info explicitly, otherwise the login screen would not be able to consume it. For keeping the compatibility, the product_info subhash under the root /api route and the hash returned by /api/product_info route is identical.

bdunne · 2018-10-12T11:35:51Z

spec/requests/entrypoint_spec.rb

+    it 'product_info contains branding_info' do
+      api_basic_authorize
+
+      expect(ActionController::Base.helpers).to receive(:image_path).at_least(2).times.and_return("foo")


Should the test above be modified or a new test added where this is not stubbed?

I'll update the test above!

bdunne · 2018-10-12T18:20:29Z

spec/requests/entrypoint_spec.rb

      )
    )
+
+    # This test will fail if you have the UI checked out and built with yarn
+    expect(response.parsed_body['product_info']).not_to include('branding_info')


It will be there, but eq({}) and the test failure reflects that

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

miq-bot · 2018-10-12T21:09:40Z

Checked commit skateman@4ff21f4 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 16 offenses detected

app/controllers/api/api_controller.rb

❗ - Line 10, Col 9 - Layout/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 11, Col 9 - Layout/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 12, Col 9 - Layout/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 13, Col 9 - Layout/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 14, Col 9 - Layout/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 15, Col 9 - Layout/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 16, Col 9 - Layout/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 24, Col 7 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 9, Col 9 - Layout/AlignHash - Align the elements of a hash literal if they span more than one line.

app/controllers/api/base_controller.rb

❗ - Line 23, Col 5 - Rails/LexicallyScopedActionFilter - product_info is not explicitly defined on the controller.
❗ - Line 23, Col 58 - Style/SymbolArray - Use %i or %I for an array of symbols.
❗ - Line 25, Col 5 - Rails/LexicallyScopedActionFilter - product_info is not explicitly defined on the controller.
❗ - Line 26, Col 5 - Rails/LexicallyScopedActionFilter - product_info is not explicitly defined on the controller.
❗ - Line 26, Col 52 - Style/SymbolArray - Use %i or %I for an array of symbols.

spec/requests/entrypoint_spec.rb

❗ - Line 91, Col 13 - Layout/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 92, Col 13 - Layout/AlignHash - Align the elements of a hash literal if they span more than one line.

simaishi · 2019-03-28T14:17:38Z

@skateman Can this be hammer/yes?

skateman · 2019-03-28T14:41:16Z

@miq-bot add_label hammer/yes

[RFE] Add the /api/product_info route with product and branding info (cherry picked from commit a40b709) https://bugzilla.redhat.com/show_bug.cgi?id=1693757

simaishi · 2019-03-29T17:41:32Z

Hammer backport details:

$ git log -1
commit 72c603072bba9201715f4ad994a5745600b39704
Author: Brandon Dunne <brandondunne@hotmail.com>
Date:   Fri Oct 12 17:52:21 2018 -0400

    Merge pull request #438 from skateman/product-info-api
    
    [RFE] Add the /api/product_info route with product and branding info
    
    (cherry picked from commit a40b7095909651f7a5814d8f4cfc934ac775bb74)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1693757

miq-bot requested a review from abellotti July 30, 2018 12:00

skateman changed the title ~~Return with product_info for an OPTIONS request on /api~~ [RFE] Return with product_info for an OPTIONS request on /api Jul 30, 2018

abellotti reviewed Jul 30, 2018

View reviewed changes

skateman force-pushed the product-info-api branch from 768f931 to da10619 Compare July 30, 2018 16:41

skateman changed the title ~~[RFE] Return with product_info for an OPTIONS request on /api~~ [RFE] Add the /api/product_info route with product and branding info Jul 30, 2018

skateman force-pushed the product-info-api branch from da10619 to e4dc9f6 Compare July 30, 2018 16:42

bdunne reviewed Jul 30, 2018

View reviewed changes

Fryguy reviewed Jul 30, 2018

View reviewed changes

skateman force-pushed the product-info-api branch from e4dc9f6 to 31220b5 Compare July 31, 2018 06:25

bdunne reviewed Jul 31, 2018

View reviewed changes

skateman force-pushed the product-info-api branch from 31220b5 to 96442f2 Compare July 31, 2018 13:50

skateman mentioned this pull request Sep 20, 2018

About modal iterates through plugins on every page load ManageIQ/manageiq-ui-classic#4581

Closed

skateman force-pushed the product-info-api branch from 96442f2 to d262868 Compare October 2, 2018 16:13

skateman mentioned this pull request Oct 2, 2018

Replace about modal with a react component that feeds on the API ManageIQ/manageiq-ui-classic#4726

Merged

skateman force-pushed the product-info-api branch from d262868 to f619326 Compare October 8, 2018 17:52

Fryguy requested changes Oct 9, 2018

View reviewed changes

skateman force-pushed the product-info-api branch from f619326 to 074acc9 Compare October 10, 2018 14:12

skateman force-pushed the product-info-api branch from 074acc9 to 9d01dbc Compare October 10, 2018 14:44

bdunne reviewed Oct 11, 2018

View reviewed changes

skateman force-pushed the product-info-api branch from 9d01dbc to 90fb60d Compare October 11, 2018 16:09

skateman force-pushed the product-info-api branch from 90fb60d to 3687b7d Compare October 12, 2018 06:57

bdunne reviewed Oct 12, 2018

View reviewed changes

skateman force-pushed the product-info-api branch from 3687b7d to 7306993 Compare October 12, 2018 15:52

bdunne reviewed Oct 12, 2018

View reviewed changes

Add the /api/product_info route with product and branding info

4ff21f4

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

skateman force-pushed the product-info-api branch from 7306993 to 4ff21f4 Compare October 12, 2018 21:04

bdunne approved these changes Oct 12, 2018

View reviewed changes

bdunne merged commit a40b709 into ManageIQ:master Oct 12, 2018

bdunne added the enhancement label Oct 12, 2018

bdunne self-assigned this Oct 12, 2018

bdunne added this to the Sprint 97 Ending Oct 22, 2018 milestone Oct 12, 2018

skateman deleted the product-info-api branch October 13, 2018 05:55

miq-bot added the hammer/yes label Mar 28, 2019

simaishi pushed a commit that referenced this pull request Mar 29, 2019

Merge pull request #438 from skateman/product-info-api

72c6030

[RFE] Add the /api/product_info route with product and branding info (cherry picked from commit a40b709) https://bugzilla.redhat.com/show_bug.cgi?id=1693757

simaishi added hammer/backported and removed hammer/yes labels Mar 29, 2019

mzazrivec mentioned this pull request Jun 20, 2019

Set product_name and product_name_full as global javascript variable ManageIQ/manageiq-ui-classic#5725

Closed

[RFE] Add the /api/product_info route with product and branding info #438

[RFE] Add the /api/product_info route with product and branding info #438

Conversation

skateman commented Jul 30, 2018 • edited Loading

juliancheal commented Jul 30, 2018

skateman commented Jul 30, 2018

juliancheal commented Jul 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skateman commented Jul 30, 2018

juliancheal commented Jul 30, 2018

skateman commented Jul 30, 2018

bdunne commented Jul 30, 2018

skateman commented Jul 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skateman commented Oct 8, 2018

bdunne commented Oct 8, 2018

skateman commented Oct 9, 2018

himdel commented Oct 9, 2018 • edited Loading

skateman commented Oct 9, 2018

Choose a reason for hiding this comment

skateman commented Oct 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skateman commented Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Oct 12, 2018

simaishi commented Mar 28, 2019

skateman commented Mar 28, 2019

simaishi commented Mar 29, 2019

skateman commented Jul 30, 2018 •

edited

Loading

himdel commented Oct 9, 2018 •

edited

Loading

skateman commented Oct 11, 2018 •

edited

Loading