Fixes #22731 - Add delete button to subscriptions page #7356

johnpmitsch · 2018-05-03T20:09:18Z

This adds the capability to delete subscriptions upstream (in RHSM) and trigger a manifest refresh.

Depends on #7317, my changes are in a separate commit to make things easier to review.

To do:

rebase once Fixes #22734 - Add ability to edit RH Subscription entitlements #7317 is merged
update tests

theforeman-bot · 2018-05-03T20:09:25Z

Issues: #22731

theforeman-bot · 2018-05-03T20:25:03Z

@johnpmitsch, this pull request is currently not mergeable. Please rebase against the master branch and push again.

If you have a remote called 'upstream' that points to this repository, you can do this by running:

    $ git pull --rebase upstream master

This message was auto-generated by Foreman's prprocessor

johnpmitsch · 2018-05-03T20:25:55Z

Note that the page won't be refreshed after the subscription is deleted, that is being handled by a separate issue which will show task progress on this page

johnpmitsch · 2018-05-03T20:28:04Z

@tstrachota @waldenraines: I'm getting this when clicking on delete, I'm at a loss to what I am passing the ConfirmDialog component that would cause this error:

warning.js:33 Warning: Received `true` for a non-boolean attribute `appear`.

If you want to write it to the DOM, pass a string instead: appear="true" or appear={value.toString()}.
    in div (created by CustomModalDialog)
    in CustomModalDialog (created by Modal)
    in Transition (created by Fade)
    in Fade (created by Modal)
    in RefHolder (created by Modal)
    in div (created by Modal)
    in Portal (created by Modal)
    in Modal (created by Modal)
    in Modal (created by ConfirmDialog)
    in ConfirmDialog (created by SubscriptionsTable)
    in Spinner (created by SubscriptionsTable)
    in SubscriptionsTable (created by SubscriptionsPage)
    in div (created by Col)
    in Col (created by SubscriptionsPage)
    in div (created by Row)
    in Row (created by SubscriptionsPage)
    in div (created by Grid)
    in Grid (created by SubscriptionsPage)
    in SubscriptionsPage (created by Connect(SubscriptionsPage))
    in Connect(SubscriptionsPage) (created by Route)
    in Route
    in div
    in Unknown (created by Application)
    in Router (created by BrowserRouter)
    in BrowserRouter (created by Application)
    in Application (created by Connect(Application))
    in Connect(Application)
    in Provider

tstrachota · 2018-05-03T21:16:54Z

webpack/scenes/Subscriptions/SubscriptionsTableSchema.js

+      },
+      formatters: [
+        label => (
+          <Table.SelectionHeading aria-label={label}>


Could you factor it out into a reusable formatter? I guess this is a pattern that will be repeated often.
Ideally we should use it on the upstream subscriptions page too.

waldenraines · 2018-05-03T21:27:41Z

I cannot test this because I always get an empty state even though I have subscriptions. I observed this on @tstrachota's PR and since that is included with this it is broken here too.

waldenraines

I can't test this because of the aforementioned empty state issue I'm seeing.

Note to any reviewers from outside the team:

The progress of the deletion will be shown once http://projects.theforeman.org/issues/23285 is implemented.

waldenraines · 2018-05-03T21:29:03Z

webpack/scenes/Subscriptions/SubscriptionsPage.js

@@ -14,6 +14,8 @@ class SubscriptionsPage extends Component {
    super(props);
    this.state = {
      manifestModalOpen: false,
+      subscriptionDeleteModalOpen: false,
+      hideDeleteButton: true,


Perhaps rename this to disableDeleteButton?

waldenraines · 2018-05-03T21:30:26Z

webpack/scenes/Subscriptions/SubscriptionsTable.js

@@ -217,6 +255,25 @@ class SubscriptionsTable extends Component {
          onConfirm={() => this.cancelEdit()}
          onCancel={() => this.showCancelConfirm(false)}
        />
+        <ConfirmDialog
+          show={this.props.subscriptionDeleteModalOpen}
+          title="Confirm Deletion"


You'll want to use {__('Confirm Deletion')} here so this is translated.

waldenraines · 2018-05-03T21:30:50Z

webpack/scenes/Subscriptions/SubscriptionsTable.js

+            ),
+          }}
+
+          confirmLabel="Delete"


Same note about translation above.

waldenraines · 2018-05-03T21:31:39Z

webpack/scenes/Subscriptions/SubscriptionsTableSchema.js

+      },
+      formatters: [
+        label => (
+          <Table.SelectionHeading aria-label={label}>


johnpmitsch · 2018-05-07T18:08:10Z

@tstrachota @waldenraines updated according to comments. A few outstanding questions:

I'm still getting this error and not sure what to do
I updated the tests and not sure if I went about it the right way, suggestions are welcome :)

johnpmitsch · 2018-05-07T18:31:25Z

to test:

enable lab features in settings
import a manifest that you own
go to lab features > subscriptions
check boxes subscriptions for subscription you want to delete, the button should be disabled if no subs are selected
click delete button
check deletion task is created, once it finishes, you will have to refresh the page to see the changes. This will be updated in a future commit that handles tasks on this page.
to add subscriptions back, find your subscription at https://access.redhat.com/management/subscription_allocations and add it back, then refresh your manifest in katello

waldenraines · 2018-05-08T02:12:29Z

I'm still getting this error and not sure what to do

I'm fine with us fixing this after this PR is merged if you want to file an issue for it.

I updated the tests and not sure if I went about it the right way, suggestions are welcome :)

We should probably do more to test individual functions within the page but we don't really do this elsewhere yet so I won't hold you to that.

waldenraines

Just a nitpick from me. Tested and it worked as expected.

waldenraines · 2018-05-08T02:19:49Z

webpack/scenes/Subscriptions/SubscriptionConstants.js

+export const DELETE_SUBSCRIPTIONS_SUCCESS = 'DELETE_SUBSCRIPTIONS_SUCCESS';
+export const DELETE_SUBSCRIPTIONS_FAILURE = 'DELETE_SUBSCRIPTIONS_FAILURE';
+
+


Extra newline here.

This will delete subscriptions upstream (in RHSM) and refresh your manifest.

johnpmitsch · 2018-05-08T14:06:56Z

@waldenraines thanks, updated

waldenraines · 2018-05-08T14:18:59Z

@johnpmitsch can you file an issue for the aforementioned JS error please? After that I think we're good to merge pending green checks.

johnpmitsch · 2018-05-08T14:27:59Z

@waldenraines filed! http://projects.theforeman.org/issues/23541

waldenraines · 2018-05-08T17:16:54Z

Feel free to merge this.

theforeman-bot added Needs testing Not yet reviewed labels May 3, 2018

waldenraines added the Blocked by another PR label May 3, 2018

theforeman-bot removed Needs testing Not yet reviewed labels May 3, 2018

theforeman-bot added the Waiting on contributor label May 3, 2018

tstrachota reviewed May 3, 2018

View reviewed changes

waldenraines suggested changes May 3, 2018

View reviewed changes

johnpmitsch force-pushed the 22731_delete_subs branch from 9f5d00d to 6b70412 Compare May 7, 2018 15:58

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels May 7, 2018

johnpmitsch force-pushed the 22731_delete_subs branch 2 times, most recently from 998c198 to cfc21af Compare May 7, 2018 18:00

johnpmitsch changed the title ~~[WIP] Fixes #22731 - Add delete button to subscriptions page~~ Fixes #22731 - Add delete button to subscriptions page May 7, 2018

Katello deleted a comment from houndci-bot May 7, 2018

johnpmitsch changed the title ~~Fixes #22731 - Add delete button to subscriptions page~~ [DO NOT MERGE] Fixes #22731 - Add delete button to subscriptions page May 7, 2018

johnpmitsch force-pushed the 22731_delete_subs branch from cfc21af to ab8e073 Compare May 7, 2018 19:07

johnpmitsch changed the title ~~[DO NOT MERGE] Fixes #22731 - Add delete button to subscriptions page~~ Fixes #22731 - Add delete button to subscriptions page May 7, 2018

johnpmitsch mentioned this pull request May 7, 2018

Fixes #23532 - Show correct upstream sub quantity #7359

Merged

waldenraines suggested changes May 8, 2018

View reviewed changes

johnpmitsch removed the Blocked by another PR label May 8, 2018

johnpmitsch force-pushed the 22731_delete_subs branch from ab8e073 to b3e31e2 Compare May 8, 2018 14:06

Fixes #22731 - Add delete button to subscriptions page

b3e31e2

This will delete subscriptions upstream (in RHSM) and refresh your manifest.

waldenraines approved these changes May 8, 2018

View reviewed changes

johnpmitsch merged commit 2984610 into Katello:master May 8, 2018

johnpmitsch deleted the 22731_delete_subs branch May 9, 2018 03:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #22731 - Add delete button to subscriptions page #7356

Fixes #22731 - Add delete button to subscriptions page #7356

johnpmitsch commented May 3, 2018 •

edited

Loading

theforeman-bot commented May 3, 2018

theforeman-bot commented May 3, 2018

johnpmitsch commented May 3, 2018

johnpmitsch commented May 3, 2018

tstrachota May 3, 2018

waldenraines May 3, 2018

waldenraines commented May 3, 2018

waldenraines left a comment

waldenraines May 3, 2018

waldenraines May 3, 2018

waldenraines May 3, 2018

waldenraines May 3, 2018

johnpmitsch commented May 7, 2018

johnpmitsch commented May 7, 2018

waldenraines commented May 8, 2018

waldenraines left a comment

waldenraines May 8, 2018

johnpmitsch commented May 8, 2018

waldenraines commented May 8, 2018 •

edited

Loading

johnpmitsch commented May 8, 2018

waldenraines commented May 8, 2018

		export const DELETE_SUBSCRIPTIONS_SUCCESS = 'DELETE_SUBSCRIPTIONS_SUCCESS';
		export const DELETE_SUBSCRIPTIONS_FAILURE = 'DELETE_SUBSCRIPTIONS_FAILURE';

Fixes #22731 - Add delete button to subscriptions page #7356

Fixes #22731 - Add delete button to subscriptions page #7356

Conversation

johnpmitsch commented May 3, 2018 • edited Loading

theforeman-bot commented May 3, 2018

theforeman-bot commented May 3, 2018

johnpmitsch commented May 3, 2018

johnpmitsch commented May 3, 2018

tstrachota May 3, 2018

Choose a reason for hiding this comment

waldenraines May 3, 2018

Choose a reason for hiding this comment

waldenraines commented May 3, 2018

waldenraines left a comment

Choose a reason for hiding this comment

waldenraines May 3, 2018

Choose a reason for hiding this comment

waldenraines May 3, 2018

Choose a reason for hiding this comment

waldenraines May 3, 2018

Choose a reason for hiding this comment

waldenraines May 3, 2018

Choose a reason for hiding this comment

johnpmitsch commented May 7, 2018

johnpmitsch commented May 7, 2018

waldenraines commented May 8, 2018

waldenraines left a comment

Choose a reason for hiding this comment

waldenraines May 8, 2018

Choose a reason for hiding this comment

johnpmitsch commented May 8, 2018

waldenraines commented May 8, 2018 • edited Loading

johnpmitsch commented May 8, 2018

waldenraines commented May 8, 2018

johnpmitsch commented May 3, 2018 •

edited

Loading

waldenraines commented May 8, 2018 •

edited

Loading