Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #31864 - Add cell formatter for entitlements column #9163

Merged
merged 3 commits into from Feb 22, 2021

Conversation

jeremylenz
Copy link
Member

Issue

While a manifest task is in progress (refresh, add subscription, delete, etc), the subscription entitlements count is showing as -1 for custom products.

Cause

The canManageSubscriptionAllocations permission is set to a falsey value during this time. (See getEntitlementsFormatter in webpack/scenes/Subscriptions/components/SubscriptionsTable/SubscriptionsTableSchema.js) This causes the formatter for the table cell to fall back to the cellFormatter instead of entitlementsInlineEditFormatter, and thus the raw value is passed directly to the table with no formatting.

To recreate

On the Subscriptions page, make sure you have at least one custom product (the Entitlement column for that product should show quantity as 'Unlimited').
Now you can see the issue one of 3 ways:

  1. Refresh the browser, and notice 'Unlimited' changes to '-1' only while the page is loading.
  2. Log in as or impersonate a user with only Viewer role. You will see '-1' even after the page is finished loading.
  3. Start a manifest task (add subscriptions, delete subscriptions, refresh manifest, etc.) You will see '-1' behind the task progress dialog.

Solution

The formatting logic should be applied regardless of whether or not the user has edit permissions. I added a new Patternfly cell formatter for the Entitlements cells, and made sure the formatting logic runs from both formatters.

@theforeman-bot
Copy link

Issues: #31864

const value = getEntitlementsDisplayValue({
rawValue, available, collapsible, upstreamPoolId,
});
const editable = (value === rawValue);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that this is the correct check. 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check aside for a moment, here's what I believe the overall functionality of this formatter should be so you can validate against that:

  • If there are multiple subscriptions (iirc we group them by the name) then show NA because the row is collapsible and will never be editable as it does not correspond to a single subscription
  • If there is an upstreamPoolId for the row (it maps to some subscription in the portal) then it should be editable
  • If the value of rawValue (I think that's the quantity added in the manifest for example?) is -1, then show Unlimited as the text. I think this only relates to:
    • custom products
    • subscriptions created 'on the fly' in local Candlepin
    • In other words these probably shouldn't be editable

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All that said, it seems like this would work for editable
const editable = !!upstreamPoolId;

And a more defensive check could be:

const editable = !!upstreamPoolId && rawValue != -1

Because subscriptions on the portal are attached in some quantity or another.

And of course you should validate what I've told you here against your own understanding and testing! If this affects the existing test cases then those should be carefully reviewed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looked good to me, but failed the test "renders unlimited for -1" because it was making 'Unlimited' editable. I changed the check to typeof value === 'number'. This should cover both NA and Unlimited, and hopefully make editable correct. let me know your thoughts.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that makes sense to me. code looks good. will test on monday!

Copy link
Member

@jturel jturel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested a number of scenarios on the subscriptions page and it all worked as expected without the anomaly mentioned in the bug report. ACK!

@jeremylenz jeremylenz merged commit 18a7153 into Katello:master Feb 22, 2021
jturel pushed a commit to jturel/katello that referenced this pull request Apr 30, 2021
…criptions page

Fixes #31864 - Add cell formatter for entitlements column (Katello#9163)

* Fixes #31864 - Add cell formatter for entitlements column

* Refs #31864 - Make Unlimited and NA not editable

* Refs #31864 - update editable check

(cherry picked from commit 18a7153)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants