Fixes #30129 - Improve experience around expired manifest identity certs #8774

jturel · 2020-06-15T23:07:22Z

This PR improves the Subscriptions UI by:

removing row selection when manifest is expired (Delete is the only action done through selection)
disabled: add subscriptions, remove subscriptions, update subscription quantity, refresh manifest
adding a tooltip to the Add Subscriptions and Refresh button
sending a notification to the UI explaining what happened

The API now raises a 400 when performing any manifest action listed above.

It also extends the existing subs expiration notifications to include checking the upstream RHSM connection with updated messaging too.

To test you'll need an older manifest with expired identity certs, and one without expired certs to make sure I didn't break anything..

For the notifications:

bundle exec rake db:seed
bundle exec rails c
Notification.destroy_all
Katello::UINotifications::Subscriptions::ManifestExpiredWarning.deliver!

theforeman-bot · 2020-06-15T23:07:28Z

Issues: #30129

app/controllers/katello/api/v2/subscriptions_controller.rb

jturel · 2020-06-15T23:18:35Z

db/seeds.d/109-katello-notification-blueprints.rb

@@ -22,7 +22,7 @@
  {
    group: N_('Subscriptions'),
    name: 'manifest_expired_warning',
-    message: N_('The manifest imported within Organization %{subject} is no longer valid.  Attempted CDN access returned Forbidden.  Refreshing the manifest may resolve this issue.'),
+    message: N_('The manifest imported within Organization %{subject} is no longer valid. Check the end date of any attached subscriptions and import a new manifest.'),


I tried to make the message little more general since there are now two scenarios when this message sent

ianballou

I've imported an older manifest of mine that I believe is expired. I'm seeing the following behavior with and without this PR checkout out:

401 Unauthorized pop-up after refreshing the page
Katello::Errors::CandlepinError: Invalid credentials. during Actions::Candlepin::Owner::UpstreamUpdate when refreshing the manifest.
Katello::UINotifications::Subscriptions::ManifestExpiredWarning.deliver! is running fine in the console but I'm not seeing a notification.

Let me know if you want to see this manifest that I'm using.

jturel · 2020-06-22T18:04:56Z

I've imported an older manifest of mine that I believe is expired. I'm seeing the following behavior with and without this PR checkout out:

401 Unauthorized pop-up after refreshing the page

Katello::Errors::CandlepinError: Invalid credentials. during Actions::Candlepin::Owner::UpstreamUpdate when refreshing the manifest.

Katello::UINotifications::Subscriptions::ManifestExpiredWarning.deliver! is running fine in the console but I'm not seeing a notification.

Let me know if you want to see this manifest that I'm using.

Sounds like you don't have the PR checked out. Assuming you do, then try restarting your rails server and killing any running Spring processes

app/models/katello/concerns/organization_extensions.rb

ianballou

Is something supposed to happen if you put in a bad CDN url? If I put google.com as my CDN I don't see any errors during a refresh. Might not be related to this PR but I did see the code was changed.

jturel · 2020-06-24T15:47:47Z

webpack/scenes/Subscriptions/SubscriptionActions.js

@@ -76,10 +74,6 @@ export const loadSubscriptions = (extendedParams = {}) => async (dispatch) => {
      response: data,
      search: extendedParams.search,
    });
-    const poolIds = filterRHSubscriptions(data.results).map(subs => subs.id);
-    if (poolIds.length > 0) {
-      dispatch(loadAvailableQuantities({ poolIds }));


I wanted to avoid making this API request if we already knew that making a connection to upstream RHSM would not be successful. Rather than hacking this method (my first approach) I decided to decouple loadAvailableQuantities and call it directly from SubscriptionsPage.

jturel · 2020-06-24T15:49:00Z

webpack/scenes/Subscriptions/SubscriptionsPage.js

@@ -56,14 +65,27 @@ class SubscriptionsPage extends Component {
        this.loadData();
      }
    }
+
+    if (hasUpstreamConnection && !prevProps.hasUpstreamConnection) {


When the page loads it issues a 'ping' (see new method in UpstreamSubscriptionsController) to see if we can reach the upstream. If that API fails, then we won't hit this block. That failure will bubble up as a single UI notification.

jturel · 2020-06-24T15:54:21Z

webpack/scenes/Subscriptions/UpstreamSubscriptions/UpstreamSubscriptionsActions.js

+  PING_UPSTREAM_SUBSCRIPTIONS_FAILURE,
+} from './UpstreamSubscriptionsConstants';
+
+export const pingUpstreamSubscriptions = () => async (dispatch) => {


I debated whether or not to have this function do anything with redux, but it seemed to fit in nicely and it is consistent with all other actions

ianballou · 2020-06-24T16:04:04Z

I noticed the tool-tip for refresh is blank when the manifest has been deleted upstream.

jturel · 2020-06-25T20:27:43Z

@ianballou this is ready to test now. I was running into some issues (merge conflicts..) and there was a Foreman change which broke the interval middleware that the Subscriptions page uses, so you'll need this PR checked out to test: theforeman/foreman#7771

The most important aspect of testing this change is making sure that the page behaves like you would expect once importing/deleting/refreshing a manifest. ie you should not be able to 'add subscriptions' if you don't have a manifest imported, or you're in disconnected mode, or you imported an expired or deleted manifest. Err on the side of caution and let me know if you see anything 'off'!

ianballou

If the manifest is bad, should we not block uploading a new one? I have to delete my manifest before uploading the good one.

Granted, the new manifest is a completely different manifest from the old one. Maybe that doesn't have to matter?

ianballou · 2020-06-25T21:51:25Z

I haven't seen any weird behavior during my testing, so I'll read through the code tomorrow and finish up my review.

jturel · 2020-06-25T22:04:45Z

If the manifest is bad, should we not block uploading a new one? I have to delete my manifest before uploading the good one.

Granted, the new manifest is a completely different manifest from the old one. Maybe that doesn't have to matter?

None of the possible manifest states prevent you from uploading a new manifest, and the only way to correct any manifest problem is to upload a new manifest (or refresh, but we're mostly talking about cases where RHSM cannot be reached in order to refresh). Not sure if I answered your question - lemme know

ianballou · 2020-06-25T22:12:32Z

@jturel my question came from noticing that, when I tried to upload a new manifest, it failed with this warning: "Import is older than existing data". It's a different manifest, but I do know that it's not expired at least. I had to delete my old manifest first. But perhaps since it's an unrelated manifest this use case doesn't matter.

jturel · 2020-06-26T13:14:16Z

@jturel my question came from noticing that, when I tried to upload a new manifest, it failed with this warning: "Import is older than existing data". It's a different manifest, but I do know that it's not expired at least. I had to delete my old manifest first. But perhaps since it's an unrelated manifest this use case doesn't matter.

Ah ok, this is not Katello behavior but Candlepin behavior. Candlepin thinks that the manifest you're trying to import is older than the currently imported one though I'm not 100% sure what that is triggered by. I'm guessing that they have some overlap in subscription data, and the data in the working manifest is older than what you already have imported.

ianballou · 2020-06-26T13:19:05Z

@jturel that makes sense, so that was my last question functionality-wise then.

…otifications

jturel · 2020-06-26T15:48:13Z

@ianballou i pushed one (hopefully) final separate commit to handle a non-obvious problem. There was a race condition where we'd try to load the 'available quantities' (this data is used when modifying the 'Entitlements' value of a Red Hat subscription on /subscriptions) before the subscriptions have loaded. Now it will load on any change in the subscriptions data (any manifest action or changing the table page) or if we haven't loaded the quantities before. However the API call is only issued if we have subscription data in the first place.

It didn't happen every page load, but you can see the problem sometimes if you watch your network tab and refresh the page with a valid manifest imported. there should be a request to to /upstream_subscriptions (vs /upstream_subscriptions/ping) but sometimes it never fired.

ianballou

Everything is looking okay to me! Functionally it's working how I expect and the code looks fine. I'm not as great at judging the JavaScript, so not sure if we need a second opinion on that. Otherwise, ACK.

jturel · 2020-06-29T13:27:19Z

Everything is looking okay to me! Functionally it's working how I expect and the code looks fine. I'm not as great at judging the JavaScript, so not sure if we need a second opinion on that. Otherwise, ACK.

Awesome! @jeremylenz would you mind giving the react code here a quick glance? SubscriptionsPage has the interesting part of the js changes

johnpmitsch · 2020-06-29T13:54:19Z

webpack/scenes/Subscriptions/SubscriptionsPage.js

@@ -56,6 +70,24 @@ class SubscriptionsPage extends Component {
      if (!prevProps.organization || prevProps.organization.id !== organization.id) {
        this.loadData();
      }
+
+      if (disconnected === false && disconnected !== prevProps.settings.disconnected) {


any reason to not use !disconnected here? if (!disconnected && disconnected !== prevProps.settings.disconnected)

When I wrote it that way I saw that disconnected was coming through as undefined (i didn't want the conditional to evaluate true when disconnected changed from undefined to false). I also had to rebase this PR on some recent changes you made which changed how the disconnected setting got loaded and where in the props it came from. I also have to cherry-pick this back into 3.16 which has the 'old' disconnected prop. For those reasons I chose to be as explicit as possible.

In order for disconnected to not start as undefined I think it needs a default state set since it's coming from redux and the default prop is basically ignored, is that right? If so, I'm hesitant to make that change here since I want to avoid more merge conflicts when I pick it to 3.16

Bet you weren't expecting such a long answer to this question! 😄

In order for disconnected to not start as undefined I think it needs a default state set since it's coming from redux and the default prop is basically ignored, is that right?

Ya that sounds likely.

Ok, that is fine, I don't see an issue checking it explicitly, we care about a boolean false value rather than a falsey one. Thanks for the explanation! 👍

@jturel Default prop-types will get set on the component regardless of Redux, so they wouldn't be ignored.

When making the connected component, Redux runs mapStateToProps and assigns props from the Redux store accordingly.

If one of those values from the Redux store isn't defined, the component won't receive that prop, and so will look to prop-types.

If prop-types has a default PropType value for that prop, it will get assigned to the component.

Even so I don't see any harm in being explicit with disconnected === false.

that makes sense. weird though - i definitely saw that disconnected was undefined. at least it works fine regardless :)

johnpmitsch · 2020-06-29T14:06:39Z

Just one small comment but the rest of the React changes look good 👍

jturel · 2020-06-29T15:00:09Z

Thanks @ianballou and @johnpmitsch ! merging

…rts (Katello#8774) * Fixes #30129 - Improve experience around expired and deleted manifests * Refs #30129 - catch all errors from upstream connection checker for notifications * Refs #30129 - ensure available quantities get loaded * Refs #30129 - don't deliver notifications to orgs without a manifest * Refs #30129 - fix candlepin resource test (cherry picked from commit 70ae87c) Conflicts: webpack/scenes/Subscriptions/SubscriptionsPage.js webpack/scenes/Subscriptions/SubscriptionsSelectors.js webpack/scenes/Subscriptions/__tests__/__snapshots__/SubscriptionsPage.test.js.snap webpack/scenes/Subscriptions/__tests__/__snapshots__/SubscriptionsReducer.test.js.snap

…rts (#8774) * Fixes #30129 - Improve experience around expired and deleted manifests * Refs #30129 - catch all errors from upstream connection checker for notifications * Refs #30129 - ensure available quantities get loaded * Refs #30129 - don't deliver notifications to orgs without a manifest * Refs #30129 - fix candlepin resource test (cherry picked from commit 70ae87c) Conflicts: webpack/scenes/Subscriptions/SubscriptionsPage.js webpack/scenes/Subscriptions/SubscriptionsSelectors.js webpack/scenes/Subscriptions/__tests__/__snapshots__/SubscriptionsPage.test.js.snap webpack/scenes/Subscriptions/__tests__/__snapshots__/SubscriptionsReducer.test.js.snap

Changes and their source: /api/hosts/:host_id/subscriptions/events is marked as deprecated in 6.7; reports dropped in https://bugzilla.redhat.com/show_bug.cgi?id=1791658; scap_content_profiles in https://github.com/theforeman/foreman_openscap/pull/431/files; override_values is in 6.7 already; template=>templates is https://projects.theforeman.org/issues/28750/ (only category name changed, path is same); status dropped in https://bugzilla.redhat.com/show_bug.cgi?id=1791656; available_virtual_machines added in https://projects.theforeman.org/issues/28548; recurring logics bulk_destroy added in theforeman/foreman-tasks#501; bulk_cancel added in theforeman/foreman-tasks#516; bulk_stop added in theforeman/foreman-tasks#543; ansible_inventories endpoints added in https://github.com/theforeman/foreman_ansible/pull/265/files; added /ansible/ prefix to ansible_inventory in https://bugzilla.redhat.com/show_bug.cgi?id=1677907; docker_tags/:id/repositories added in Katello/katello#8658; /api/hosts/:host_id/traces/resolve added in Katello/katello#8945; autoattach_subscriptions marked as deprecated in 6.7; /katello/api/subscriptions was there in 6.7; template_combinations deprecated and changed to different paths; upstream_subscriptions added in Katello/katello#8774; https://bugzilla.redhat.com/show_bug.cgi?id=1887932 workarounded; mail_notifications added in theforeman/foreman#7549

theforeman-bot added Waiting on contributor Needs testing Not yet reviewed labels Jun 15, 2020

jturel commented Jun 15, 2020

View reviewed changes

app/controllers/katello/api/v2/subscriptions_controller.rb Outdated Show resolved Hide resolved

jturel commented Jun 15, 2020

View reviewed changes

jturel removed the Waiting on contributor label Jun 16, 2020

ianballou reviewed Jun 22, 2020

View reviewed changes

app/models/katello/concerns/organization_extensions.rb Show resolved Hide resolved

ianballou reviewed Jun 22, 2020

View reviewed changes

jturel force-pushed the expired_manifest branch from 1b5c408 to 5318e10 Compare June 24, 2020 15:43

theforeman-bot added the Waiting on contributor label Jun 24, 2020

jturel commented Jun 24, 2020

View reviewed changes

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Jun 24, 2020

jturel force-pushed the expired_manifest branch from d735dcb to 9191b64 Compare June 24, 2020 21:50

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Jun 24, 2020

Fixes #30129 - Improve experience around expired and deleted manifests

bf6ef06

jturel force-pushed the expired_manifest branch from 9191b64 to bf6ef06 Compare June 25, 2020 20:21

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Jun 25, 2020

ianballou reviewed Jun 25, 2020

View reviewed changes

jturel removed the Waiting on contributor label Jun 26, 2020

Refs #30129 - catch all errors from upstream connection checker for n…

8eb9cb9

…otifications

theforeman-bot added the Waiting on contributor label Jun 26, 2020

Refs #30129 - ensure available quantities get loaded

d403b71

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Jun 26, 2020

jturel added 2 commits June 26, 2020 18:31

Refs #30129 - don't deliver notifications to orgs without a manifest

b873334

Refs #30129 - fix candlepin resource test

2916a22

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Jun 26, 2020

ianballou approved these changes Jun 26, 2020

View reviewed changes

johnpmitsch reviewed Jun 29, 2020

View reviewed changes

jturel merged commit 70ae87c into Katello:master Jun 29, 2020

jturel deleted the expired_manifest branch June 29, 2020 15:00

lhellebr mentioned this pull request Oct 14, 2020

Changing expected API endpoints. SatelliteQE/robottelo#8058

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #30129 - Improve experience around expired manifest identity certs #8774

Fixes #30129 - Improve experience around expired manifest identity certs #8774

jturel commented Jun 15, 2020 •

edited

theforeman-bot commented Jun 15, 2020

jturel Jun 15, 2020

ianballou left a comment

jturel commented Jun 22, 2020

ianballou left a comment

jturel Jun 24, 2020

jturel Jun 24, 2020

jturel Jun 24, 2020

ianballou commented Jun 24, 2020

jturel commented Jun 25, 2020

ianballou left a comment •

edited

ianballou commented Jun 25, 2020

jturel commented Jun 25, 2020

ianballou commented Jun 25, 2020 •

edited

jturel commented Jun 26, 2020

ianballou commented Jun 26, 2020

jturel commented Jun 26, 2020

ianballou left a comment

jturel commented Jun 29, 2020

johnpmitsch Jun 29, 2020

jturel Jun 29, 2020

johnpmitsch Jun 29, 2020

jeremylenz Jun 29, 2020

jturel Jun 29, 2020

johnpmitsch commented Jun 29, 2020

jturel commented Jun 29, 2020

Fixes #30129 - Improve experience around expired manifest identity certs #8774

Fixes #30129 - Improve experience around expired manifest identity certs #8774

Conversation

jturel commented Jun 15, 2020 • edited

theforeman-bot commented Jun 15, 2020

Choose a reason for hiding this comment

ianballou left a comment

Choose a reason for hiding this comment

jturel commented Jun 22, 2020

ianballou left a 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

ianballou commented Jun 24, 2020

jturel commented Jun 25, 2020

ianballou left a comment • edited

Choose a reason for hiding this comment

ianballou commented Jun 25, 2020

jturel commented Jun 25, 2020

ianballou commented Jun 25, 2020 • edited

jturel commented Jun 26, 2020

ianballou commented Jun 26, 2020

jturel commented Jun 26, 2020

ianballou left a comment

Choose a reason for hiding this comment

jturel commented Jun 29, 2020

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

johnpmitsch commented Jun 29, 2020

jturel commented Jun 29, 2020

jturel commented Jun 15, 2020 •

edited

ianballou left a comment •

edited

ianballou commented Jun 25, 2020 •

edited