Fixes #27605 - Adding generic first row for grouped subs #8336

sseelam2 · 2019-09-06T16:56:34Z

This PR is still a work in progess. I would appreciate suggestions.
This PR is changing the view of the subscriptions with multiple entitlements.

To test this PR -

Create a manifest with a subscription having multiple entitlements in the Red Hat customer portal.
Click on manage manifest and import this manifest under the Subscriptions tab.
When this is viewed in the UI (Content -> Subscriptions), the first row for this subscription must be collapsible having the subscription name and product ID and everything else set to NA. When '>' is clicked it should show all the subscriptions under the generic row with the same product ID.

theforeman-bot · 2019-09-06T17:04:07Z

Issues: #27605

jturel · 2019-09-09T20:21:04Z

@sseelam2 I saw this was in ready for review but it appears to be a work in progress. Was there something specific you're looking for feedback on?

sseelam2 · 2019-09-09T20:25:17Z

@jturel Sorry I forgot to mention that. I still have not figured out how to make the entitlements column NA for the generic row. And also I'm not sure why '>' this symbol comes up in the first two rows when it should be present only on the first row.

jturel · 2019-09-09T20:35:22Z

@sseelam2 that's OK. I am also curious if you talked with anyone from the UX side on this. Maybe it's better to not collapse/group the subscriptions, but instead list them individually. That makes it easier from our side but I think I can also see why we have the grouping.

sseelam2 · 2019-09-09T20:45:26Z

@jturel I discussed with Roxanne about this. She went through the description of the bug and mentioned that she had seen something just like this earlier and asked me to go ahead with this solution.

webpack/scenes/Subscriptions/components/SubscriptionsTable/SubscriptionsTableHelpers.js

jlsherrill · 2019-09-16T13:39:00Z

Can we make the commit message a little more descriptive?

sseelam2 · 2019-09-17T13:24:19Z

@jlsherrill We are not sure how we should be displaying the subscriptions yet. @jturel and I have a meeting with Roxanne this friday to figure out a better way of displaying the subscriptions.

sseelam2 · 2019-09-24T13:53:08Z

Can we make the commit message a little more descriptive?

I have edited the commit message. Is there anything else that I need to be adding? @jlsherrill

jturel · 2019-09-24T16:54:48Z

webpack/scenes/Subscriptions/components/SubscriptionsTable/components/Table.js

@@ -27,7 +27,7 @@ const Table = ({
  const groupingController = {
    isCollapseable: ({ rowData }) =>
      // it is the first subscription in the group
-      rowData.id === groupedSubscriptions[rowData.product_id].subscriptions[0].id &&
+      rowData.id === 0 &&


How is checking the id is 0 significant?

Oh, I think it's so we don't get the '>' on the expanded rows also?

@jturel yes that's exactly why.

I wonder if there's some other distinguishing characteristic about the row (maybe something else we can add) to identify when we should collapse rather than id === 0. What if we had an Id later? That would break the functionality because it's not obvious that ID is significant in that way.

What about putting a 'collapsible' field on the custom row we are building?

@jturel Subscriptions already have an id field which starts from 1. So I just used id=0 to represent a generic row. But using a collapsible field sounds good to me.

webpack/scenes/Subscriptions/components/SubscriptionsTable/components/Table.js

jturel · 2019-10-07T15:35:05Z

@sseelam2 looks like the build is green now. is this ready for re-review? just checking in case you were planning any more changes

sseelam2 · 2019-10-07T15:36:31Z

@sseelam2 looks like the build is green now. is this ready for re-review? just checking in case you were planning any more changes

Yes, It is ready for re-review. 👍

jturel · 2019-10-07T15:56:52Z

@jeremylenz hey! I'll take care of the end-to-end testing of the PR. Would you mind looking at it from the JS perspective and make sure that what we came up with isn't breaking any rules?

jeremylenz

Thanks @sseelam2, looking good!

Left a few code comments below.

jeremylenz · 2019-10-07T18:41:25Z

webpack/move_to_foreman/components/common/table/components/TableSelectionCell.js

 }) => (
  <Table.SelectionCell>
    {before}
-    <Table.Checkbox
+    {!hide && <Table.Checkbox


My favorite way to conditionally render :)

I think it's more readable if <Table.Checkbox is on the next line.

alright. thanks for the feedback. will move it to the next line. 👍

jeremylenz · 2019-10-07T18:42:55Z

.../move_to_foreman/components/common/table/formatters/collapseableAndSelectionCellFormatter.js

@@ -4,8 +4,10 @@ import CollapseSubscriptionGroupButton from '../components/CollapseSubscriptionG

 export default (collapseableController, selectionController, additionalData) => {
  const shouldShowCollapseButton = collapseableController.isCollapseable(additionalData);
+  const isGenericRow = additionalData.rowData.collapsible;


If additionalData.rowData is ever undefined, this will throw an error. You can leave as-is if you're confident that rowData will always be defined, but it's something to keep in mind..

The table rows are built only if there are any subscriptions and these rows are passed in as additional data to collapseableAndSelectionCellFormatter.js . So I don't think addtionalData can be empty. Correct me if I'm wrong @jturel.

@sseelam2 rowData is the one at issue. If additionalData is defined but additionalData.rowData isn't, it will error.

additonalData doesn't get created if there is no rowData. additonalData contains additional info about the row. So I don't think we'll end up in that situation. But I would like @jturel to confirm. Just to be extra sure about this.

That sounds correct :)

jeremylenz · 2019-10-07T18:47:09Z

webpack/scenes/Subscriptions/components/SubscriptionsTable/SubscriptionsTableHelpers.js

+  const first = subscriptionGroup.subscriptions[0];
+  const heading = {};
+
+  heading.id = 0;


Unless there's a specific reason for assigning all the properties using =, I think an object literal is more readable:

const heading = { id: 0, collapsible: true, ...

This makes sense. Thanks 👍

jeremylenz · 2019-10-07T18:48:14Z

webpack/scenes/Subscriptions/components/SubscriptionsTable/SubscriptionsTableHelpers.js

+  heading.contract_number = 'NA';
+  heading.start_date = 'NA';
+  heading.end_date = 'NA';
+  heading.consumed = 'NA';
+  heading.product_id = first.product_id;
+  heading.name = first.name;
+  heading.virt_only = first.virt_only;


Is it possible to use camelCase for all these instead of snake_case?

jeremylenz · 2019-10-07T19:06:05Z

webpack/scenes/Subscriptions/components/SubscriptionsTable/SubscriptionsTableHelpers.js

+  if (subscriptions.length > 1) {
+    rows.push(buildTableCollapseRow(subscriptionGroup));
+    if (open) {
+      return rows.concat(subscriptions.map(sub =>


What about, instead of return rows.concat if we were to use subscriptions.forEach(sub => rows.push(... ?

That way, on line 65 you're just returning rows no matter what, and it's a little easier to see what's happening.

Nice catch. Thanks for your feedback @jeremylenz .

sseelam2 · 2019-10-09T20:13:33Z

@jturel PR is ready for testing :)

jturel · 2019-10-10T20:55:00Z

webpack/move_to_foreman/components/common/table/formatters/EntitlementsInlineEditFormatter.js

@@ -6,11 +6,17 @@ import { validateQuantity } from '../../../../../scenes/Subscriptions/Subscripti

 const renderValue = (value, additionalData, onActivate) => {
  const { available, upstream_pool_id: upstreamPoolId } = additionalData.rowData;
-  if (available < 0 || !upstreamPoolId) {
+
+  if (additionalData.rowData.collapsible) {


Can we pull out collapsible via destructuring on line 8? What's here works fine, but that would be more consistent with the surrounding code

jturel · 2019-10-10T20:56:07Z

webpack/scenes/Subscriptions/components/SubscriptionsTable/SubscriptionsTableHelpers.js

 const buildTableRowsFromGroup = (subscriptionGroup, availableQuantities, updatedQuantity) => {
  const { open, subscriptions } = subscriptionGroup;
+  const rows = [];


I think rows can be initialized inside the below if statement, so that we don't initialize it unnecessarily

jturel

Left 2 small comments, but it works exactly how I imagined it should!

sseelam2 · 2019-10-10T22:11:33Z

Thanks @jturel and @jeremylenz for the feedback! 😄

jturel

ACK!

Thanks @sseelam2 and @jeremylenz for the review

theforeman-bot added Needs testing Not yet reviewed labels Sep 6, 2019

sseelam2 force-pushed the manifest_bug branch from f0c4822 to e40f9dc Compare September 6, 2019 17:09

jturel self-assigned this Sep 9, 2019

jturel reviewed Sep 9, 2019

View reviewed changes

webpack/scenes/Subscriptions/components/SubscriptionsTable/SubscriptionsTableHelpers.js Outdated Show resolved Hide resolved

sseelam2 force-pushed the manifest_bug branch 2 times, most recently from 3c8ce1a to cd7633f Compare September 10, 2019 15:05

jturel reviewed Sep 10, 2019

View reviewed changes

webpack/scenes/Subscriptions/components/SubscriptionsTable/SubscriptionsTableHelpers.js Outdated Show resolved Hide resolved

sseelam2 force-pushed the manifest_bug branch 2 times, most recently from ca16699 to b0d0ce4 Compare September 10, 2019 17:43

jturel reviewed Sep 10, 2019

View reviewed changes

webpack/scenes/Subscriptions/components/SubscriptionsTable/SubscriptionsTableHelpers.js Outdated Show resolved Hide resolved

jturel changed the title ~~Fixes #27605 - Mainfest Issue~~ [WIP] Fixes #27605 - Mainfest Issue Sep 19, 2019

sseelam2 force-pushed the manifest_bug branch from b0d0ce4 to 98caf3d Compare September 24, 2019 14:57

jturel reviewed Sep 24, 2019

View reviewed changes

sseelam2 force-pushed the manifest_bug branch 6 times, most recently from a87d2fe to 31b7961 Compare September 26, 2019 15:32

sseelam2 changed the title ~~[WIP] Fixes #27605 - Mainfest Issue~~ Fixes #27605 - Mainfest Issue Sep 26, 2019

jturel reviewed Sep 27, 2019

View reviewed changes

webpack/scenes/Subscriptions/components/SubscriptionsTable/components/Table.js Outdated Show resolved Hide resolved

sseelam2 force-pushed the manifest_bug branch 12 times, most recently from 25a7600 to 8edc4ba Compare October 4, 2019 21:54

jeremylenz reviewed Oct 7, 2019

View reviewed changes

sseelam2 force-pushed the manifest_bug branch 2 times, most recently from 9394735 to eeb2abd Compare October 9, 2019 18:02

sseelam2 changed the title ~~Fixes #27605 - Mainfest Issue~~ Fixes #27605 - Adding generic first row for grouped subs Oct 9, 2019

jturel reviewed Oct 10, 2019

View reviewed changes

jturel suggested changes Oct 10, 2019

View reviewed changes

sseelam2 force-pushed the manifest_bug branch from eeb2abd to 13654d3 Compare October 10, 2019 22:00

Fixes #27605 - Adding generic first row for grouped subs

a17056b

sseelam2 force-pushed the manifest_bug branch from 13654d3 to a17056b Compare October 10, 2019 22:07

jturel approved these changes Oct 11, 2019

View reviewed changes

jturel merged commit 91c3240 into Katello:master Oct 11, 2019

sseelam2 deleted the manifest_bug branch October 11, 2019 14:40

Fixes #27605 - Adding generic first row for grouped subs #8336

Fixes #27605 - Adding generic first row for grouped subs #8336

Conversation

sseelam2 commented Sep 6, 2019 • edited

theforeman-bot commented Sep 6, 2019

jturel commented Sep 9, 2019

sseelam2 commented Sep 9, 2019

jturel commented Sep 9, 2019

sseelam2 commented Sep 9, 2019

jlsherrill commented Sep 16, 2019

sseelam2 commented Sep 17, 2019

sseelam2 commented Sep 24, 2019

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

jturel commented Oct 7, 2019

sseelam2 commented Oct 7, 2019

jturel commented Oct 7, 2019

jeremylenz 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

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

sseelam2 commented Oct 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jturel left a comment

Choose a reason for hiding this comment

sseelam2 commented Oct 10, 2019

jturel left a comment

Choose a reason for hiding this comment

sseelam2 commented Sep 6, 2019 •

edited