Fixes #23286 - Add confirmation dialog on delete manifest #7365

johnpmitsch · 2018-05-10T20:59:07Z

No description provided.

theforeman-bot · 2018-05-10T20:59:13Z

Issues: #23286

theforeman-bot · 2018-05-10T20:59:14Z

There were the following issues with the commit message:

length of the first commit message line for 8e3b976 exceeds 65 characters

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

houndci-bot · 2018-05-10T20:59:50Z

webpack/scenes/Subscriptions/Manifest/ManageManifestModal.js

+                      onConfirm={() => this.hideModal()}
+                      onCancel={() => this.showCancelConfirm(false)}
+                    />
+


Trailing spaces not allowed no-trailing-spaces

johnpmitsch · 2018-05-10T21:03:53Z

To review: go to new subscriptions page and click "manage manifest" button. Close the modal and you should see another modal confirming you want to exit the modal.

johnpmitsch · 2018-05-10T21:05:56Z

@waldenraines @tstrachota: one loophole I found with this change is that you can click outside of a modal to dismiss it, which will just exit and wont trigger the "confirm you want to exit" dialog modal.

johnpmitsch · 2018-05-10T21:25:20Z

I misunderstood the issue, marking this WIP while I update correctly. Please disregard my earlier question :)

waldenraines · 2018-05-10T22:29:19Z

As discussed on IRC this modal is supposed to be displayed when delete is clicked to confirm the manifest deletion itself. There is actually no way to not save your changes from within the modal itself since everything is saved on change.

houndci-bot · 2018-05-11T18:16:28Z

webpack/scenes/Subscriptions/Manifest/ManageManifestModal.js

+                      }}
+                      confirmLabel={__('Delete')}
+                      confirmStyle={"danger"}
+                      onConfirm={() => this.deleteManifest() }


There should be no space before '}' react/jsx-curly-spacing

houndci-bot · 2018-05-11T18:16:28Z

webpack/scenes/Subscriptions/Manifest/ManageManifestModal.js

+                          )
+                      }}
+                      confirmLabel={__('Delete')}
+                      confirmStyle={"danger"}


Curly braces are unnecessary here react/jsx-curly-brace-presence
Strings must use singlequote quotes

houndci-bot · 2018-05-11T18:16:28Z

webpack/scenes/Subscriptions/Manifest/ManageManifestModal.js

+                      dangerouslySetInnerHTML={{
+                        __html: sprintf(
+                          __(DeleteManifestModalText)
+                          )


Unexpected newline before ')' function-paren-newline
Missing trailing comma comma-dangle

houndci-bot · 2018-05-11T18:16:28Z

webpack/scenes/Subscriptions/Manifest/ManageManifestModal.js

+                      title={__('Confirm delete manifest')}
+                      dangerouslySetInnerHTML={{
+                        __html: sprintf(
+                          __(DeleteManifestModalText)


Missing trailing comma comma-dangle

houndci-bot · 2018-05-11T18:16:28Z

webpack/scenes/Subscriptions/Manifest/ManageManifestModal.js

+                      show={this.state.showDeleteManifestModalDialog}
+                      title={__('Confirm delete manifest')}
+                      dangerouslySetInnerHTML={{
+                        __html: sprintf(


Unexpected newline after '(' function-paren-newline

houndci-bot · 2018-05-11T18:16:28Z

webpack/scenes/Subscriptions/Manifest/ManageManifestModal.js

@@ -56,6 +60,13 @@ class ManageManifestModal extends Component {

  deleteManifest() {
    this.props.deleteManifest().then(this.props.loadOrganization);
+    this.showDeleteManifestModal(false)


Missing semicolon semi

houndci-bot · 2018-05-11T18:16:28Z

webpack/scenes/Subscriptions/Manifest/ManageManifestModal.js

@@ -5,13 +5,17 @@ import { bindMethods, Alert, Button, Icon, Modal, ProgressBar, Spinner, OverlayT
 import TooltipButton from 'react-bootstrap-tooltip-button';
 import { Table } from '../../../move_to_foreman/components/common/table';
 import { columns } from './ManifestHistoryTableSchema';
+import ConfirmDialog from '../../../move_to_foreman/components/common/ConfirmDialog';
+import DeleteManifestModalText from "./DeleteManifestModalText";
+import { sprintf } from 'jed';


Absolute imports should come before relative imports import/first

houndci-bot · 2018-05-11T18:16:28Z

webpack/scenes/Subscriptions/Manifest/ManageManifestModal.js

@@ -5,13 +5,17 @@ import { bindMethods, Alert, Button, Icon, Modal, ProgressBar, Spinner, OverlayT
 import TooltipButton from 'react-bootstrap-tooltip-button';
 import { Table } from '../../../move_to_foreman/components/common/table';
 import { columns } from './ManifestHistoryTableSchema';
+import ConfirmDialog from '../../../move_to_foreman/components/common/ConfirmDialog';
+import DeleteManifestModalText from "./DeleteManifestModalText";


Strings must use singlequote quotes

houndci-bot · 2018-05-11T18:16:29Z

webpack/move_to_foreman/components/common/ConfirmDialog/ConfirmDialog.js

@@ -43,6 +44,7 @@ ConfirmDialog.defaultProps = {
  cancelLabel: __('Cancel'),
  dangerouslySetInnerHTML: undefined,
  message: undefined,
+  confirmStyle: "primary",


Strings must use singlequote quotes

houndci-bot · 2018-05-11T18:16:29Z

webpack/scenes/Subscriptions/Manifest/ManageManifestModal.js

+                      }}
+                      confirmLabel={__('Delete')}
+                      confirmStyle={"danger"}
+                      onConfirm={() => this.deleteManifest() }


There should be no space before '}' react/jsx-curly-spacing

houndci-bot · 2018-05-11T18:16:29Z

webpack/scenes/Subscriptions/Manifest/ManageManifestModal.js

+                          )
+                      }}
+                      confirmLabel={__('Delete')}
+                      confirmStyle={"danger"}


Curly braces are unnecessary here react/jsx-curly-brace-presence
Strings must use singlequote quotes

houndci-bot · 2018-05-11T18:16:29Z

webpack/scenes/Subscriptions/Manifest/ManageManifestModal.js

+                      dangerouslySetInnerHTML={{
+                        __html: sprintf(
+                          __(DeleteManifestModalText)
+                          )


Unexpected newline before ')' function-paren-newline
Missing trailing comma comma-dangle

houndci-bot · 2018-05-11T18:16:30Z

webpack/scenes/Subscriptions/Manifest/ManageManifestModal.js

+                      title={__('Confirm delete manifest')}
+                      dangerouslySetInnerHTML={{
+                        __html: sprintf(
+                          __(DeleteManifestModalText)


Missing trailing comma comma-dangle

houndci-bot · 2018-05-11T18:16:30Z

webpack/scenes/Subscriptions/Manifest/ManageManifestModal.js

+                      show={this.state.showDeleteManifestModalDialog}
+                      title={__('Confirm delete manifest')}
+                      dangerouslySetInnerHTML={{
+                        __html: sprintf(


Unexpected newline after '(' function-paren-newline

houndci-bot · 2018-05-11T18:16:30Z

webpack/scenes/Subscriptions/Manifest/ManageManifestModal.js

@@ -56,6 +60,13 @@ class ManageManifestModal extends Component {

  deleteManifest() {
    this.props.deleteManifest().then(this.props.loadOrganization);
+    this.showDeleteManifestModal(false)


Missing semicolon semi

houndci-bot · 2018-05-11T18:16:30Z

webpack/scenes/Subscriptions/Manifest/ManageManifestModal.js

@@ -5,13 +5,17 @@ import { bindMethods, Alert, Button, Icon, Modal, ProgressBar, Spinner, OverlayT
 import TooltipButton from 'react-bootstrap-tooltip-button';
 import { Table } from '../../../move_to_foreman/components/common/table';
 import { columns } from './ManifestHistoryTableSchema';
+import ConfirmDialog from '../../../move_to_foreman/components/common/ConfirmDialog';
+import DeleteManifestModalText from "./DeleteManifestModalText";
+import { sprintf } from 'jed';


Absolute imports should come before relative imports import/first

houndci-bot · 2018-05-11T18:16:30Z

webpack/scenes/Subscriptions/Manifest/ManageManifestModal.js

@@ -5,13 +5,17 @@ import { bindMethods, Alert, Button, Icon, Modal, ProgressBar, Spinner, OverlayT
 import TooltipButton from 'react-bootstrap-tooltip-button';
 import { Table } from '../../../move_to_foreman/components/common/table';
 import { columns } from './ManifestHistoryTableSchema';
+import ConfirmDialog from '../../../move_to_foreman/components/common/ConfirmDialog';
+import DeleteManifestModalText from "./DeleteManifestModalText";


Strings must use singlequote quotes

houndci-bot · 2018-05-11T18:16:30Z

webpack/move_to_foreman/components/common/ConfirmDialog/ConfirmDialog.js

@@ -43,6 +44,7 @@ ConfirmDialog.defaultProps = {
  cancelLabel: __('Cancel'),
  dangerouslySetInnerHTML: undefined,
  message: undefined,
+  confirmStyle: "primary",


Strings must use singlequote quotes

johnpmitsch · 2018-05-11T18:23:35Z

@waldenraines I found the refresh/delete buttons were still enabled when there is no manifest imported, I filed an issue here: http://projects.theforeman.org/issues/23571

johnpmitsch · 2018-05-11T18:24:54Z

@tstrachota @waldenraines: updated the modal to show on manifest delete. I'm using a large JSX block inside the modal, I'm not sure what the best way to handle translating that is?

tstrachota · 2018-05-14T11:38:52Z

webpack/scenes/Subscriptions/Manifest/ManageManifestModal.js

+                      show={this.state.showDeleteManifestModalDialog}
+                      title={__('Confirm delete manifest')}
+                      dangerouslySetInnerHTML={{
+                        __html: sprintf(__(DeleteManifestModalText)),


I'm afraid this is not going to work. AFAIK code isn't evaluated when the translations are being extracted.

BTW sprintf is useless here.

tstrachota · 2018-05-14T11:41:12Z

webpack/scenes/Subscriptions/Manifest/DeleteManifestModalText.js

+  <li>Disable Red Hat Insights</li>
+  <li>Require you to upload the subscription-manifest and re-attach subscriptions to hosts and activation keys.</li>
+</ul>
+<p>This action should only be taken in extreme circumstances or for debugging purposes.</p>`;


How about splitting it into three parts: "Are you sure..", "Note: ..." and "This action..." to get at least the paragraphs out of the translated text.

houndci-bot · 2018-05-14T14:55:15Z

webpack/scenes/Subscriptions/Manifest/DeleteManifestModalText.js

+const l3 = __("Disable Red Hat Insights.");
+const l4 = __("Require you to upload the subscription-manifest and re-attach \
+               subscriptions to hosts and activation keys.");
+const debug = __("This action should only be taken in extreme circumstances or \


Multiline support is limited to browsers supporting ES5 only no-multi-str
Strings must use singlequote quotes

houndci-bot · 2018-05-14T14:55:15Z

webpack/scenes/Subscriptions/Manifest/DeleteManifestModalText.js

+const l1 = __("Delete all subscriptions that are attached to running hosts.");
+const l2 = __("Delete all subscriptions attached to activation keys.");
+const l3 = __("Disable Red Hat Insights.");
+const l4 = __("Require you to upload the subscription-manifest and re-attach \


Multiline support is limited to browsers supporting ES5 only no-multi-str
Strings must use singlequote quotes

houndci-bot · 2018-05-14T14:55:15Z

webpack/scenes/Subscriptions/Manifest/DeleteManifestModalText.js

+                 Deleting a manifest will:");
+const l1 = __("Delete all subscriptions that are attached to running hosts.");
+const l2 = __("Delete all subscriptions attached to activation keys.");
+const l3 = __("Disable Red Hat Insights.");


Strings must use singlequote quotes

houndci-bot · 2018-05-14T14:55:15Z

webpack/scenes/Subscriptions/Manifest/DeleteManifestModalText.js

+const note = __("Note: Deleting a subscription manifest is STRONGLY discouraged. \
+                 Deleting a manifest will:");
+const l1 = __("Delete all subscriptions that are attached to running hosts.");
+const l2 = __("Delete all subscriptions attached to activation keys.");


Strings must use singlequote quotes

houndci-bot · 2018-05-14T14:55:15Z

webpack/scenes/Subscriptions/Manifest/DeleteManifestModalText.js

+const question = __("Are you sure you want to delete the manifest?");
+const note = __("Note: Deleting a subscription manifest is STRONGLY discouraged. \
+                 Deleting a manifest will:");
+const l1 = __("Delete all subscriptions that are attached to running hosts.");


Strings must use singlequote quotes

houndci-bot · 2018-05-14T14:55:16Z

webpack/scenes/Subscriptions/Manifest/DeleteManifestModalText.js

@@ -0,0 +1,22 @@
+const question = __("Are you sure you want to delete the manifest?");
+const note = __("Note: Deleting a subscription manifest is STRONGLY discouraged. \


Multiline support is limited to browsers supporting ES5 only no-multi-str
Strings must use singlequote quotes

houndci-bot · 2018-05-14T14:55:16Z

webpack/scenes/Subscriptions/Manifest/DeleteManifestModalText.js

@@ -0,0 +1,22 @@
+const question = __("Are you sure you want to delete the manifest?");


Strings must use singlequote quotes

johnpmitsch · 2018-05-14T15:02:52Z

Sorry about the hound notifications, I'm having trouble running npm run test locally

houndci-bot · 2018-05-14T15:34:44Z

webpack/scenes/Subscriptions/Manifest/ManageManifestModal.js

@@ -3,15 +3,19 @@ import PropTypes from 'prop-types';
 import { Col, Tabs, Tab, Form, FormGroup, FormControl, ControlLabel } from 'react-bootstrap';
 import { bindMethods, Alert, Button, Icon, Modal, ProgressBar, Spinner, OverlayTrigger, Tooltip } from 'patternfly-react';
 import TooltipButton from 'react-bootstrap-tooltip-button';
+import { sprintf } from 'jed';


'sprintf' is defined but never used no-unused-vars

johnpmitsch · 2018-05-14T15:37:18Z

@waldenraines @tstrachota I updated to translate each string and then use it in the JSX, I'm not sure if that is the best solution but I'm open to suggestions 😸

waldenraines · 2018-05-14T18:13:53Z

@waldenraines I found the refresh/delete buttons were still enabled when there is no manifest imported, I filed an issue here: http://projects.theforeman.org/issues/23571

Nice catch.

waldenraines

@waldenraines @tstrachota I updated to translate each string and then use it in the JSX, I'm not sure if that is the best solution but I'm open to suggestions

Responded to this with a code comment below.

waldenraines · 2018-05-14T18:23:04Z

webpack/scenes/Subscriptions/Manifest/DeleteManifestModalText.js

+const DeleteManifestModalText = `<p>${question}</p>
+<p>${note}</p>
+<ul class="list-aligned">
+  <li>${l1}</li>


Would something like this work?

<li>{__('Delete all subscriptions that are attached to running hosts.')}</li>

It looks like this is actually being evaluated as html so that won't work. When I tried <p>{__('does this work?')}</p>, it looks like this:

It makes sense since we are passing to dangerouslySetInnerHTML

It makes sense since we are passing to dangerouslySetInnerHTML

Oh, right. And same thing with this?

<p>${__('does this work?')}</p>

Hmm, I wonder if we should update the Dialog component to accept children too. That would probably solve the problem because we could easily pass array of components instead of a string.

That's a good thought but I'd want to do it outside of this PR. Since the above works I am fine merging as is and then fixing this later. See http://projects.theforeman.org/issues/23600

waldenraines · 2018-05-14T23:28:37Z

[test katello]

waldenraines · 2018-05-15T14:55:18Z

[test katello]

theforeman-bot added Waiting on contributor Needs testing Not yet reviewed labels May 10, 2018

houndci-bot reviewed May 10, 2018

View reviewed changes

johnpmitsch force-pushed the 23286_delete_confirmation branch from 8e3b976 to 1146364 Compare May 10, 2018 21:03

theforeman-bot added Needs testing and removed Waiting on contributor labels May 10, 2018

johnpmitsch force-pushed the 23286_delete_confirmation branch from 1146364 to ebb9056 Compare May 11, 2018 18:16

houndci-bot reviewed May 11, 2018

View reviewed changes

johnpmitsch force-pushed the 23286_delete_confirmation branch from ebb9056 to 1258f1b Compare May 11, 2018 18:21

johnpmitsch force-pushed the 23286_delete_confirmation branch from 1258f1b to 0f2f334 Compare May 11, 2018 18:44

johnpmitsch changed the title ~~Fixes #23286 - Add confirmation dialog on manage subscription modal~~ Fixes #23286 - Add confirmation dialog on delete manifest May 11, 2018

tstrachota reviewed May 14, 2018

View reviewed changes

johnpmitsch force-pushed the 23286_delete_confirmation branch from 0f2f334 to ed3babd Compare May 14, 2018 14:54

houndci-bot reviewed May 14, 2018

View reviewed changes

johnpmitsch force-pushed the 23286_delete_confirmation branch 2 times, most recently from 36a9922 to 24344d5 Compare May 14, 2018 15:34

houndci-bot reviewed May 14, 2018

View reviewed changes

johnpmitsch force-pushed the 23286_delete_confirmation branch from 24344d5 to 8ab296a Compare May 14, 2018 15:36

Fixes #23286 - Add confirmation dialog for deleting manifest

8ab296a

waldenraines suggested changes May 14, 2018

View reviewed changes

waldenraines approved these changes May 15, 2018

View reviewed changes

waldenraines merged commit e8ff2f4 into Katello:master May 15, 2018

johnpmitsch deleted the 23286_delete_confirmation branch May 16, 2018 05:08

		@@ -0,0 +1,22 @@
		const question = __("Are you sure you want to delete the manifest?");
		const note = __("Note: Deleting a subscription manifest is STRONGLY discouraged. \

Fixes #23286 - Add confirmation dialog on delete manifest #7365

Fixes #23286 - Add confirmation dialog on delete manifest #7365

Conversation

johnpmitsch commented May 10, 2018

theforeman-bot commented May 10, 2018

theforeman-bot commented May 10, 2018

Choose a reason for hiding this comment

johnpmitsch commented May 10, 2018

johnpmitsch commented May 10, 2018

johnpmitsch commented May 10, 2018

waldenraines commented May 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

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

johnpmitsch commented May 11, 2018

johnpmitsch commented May 11, 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

johnpmitsch commented May 14, 2018

Choose a reason for hiding this comment

johnpmitsch commented May 14, 2018

waldenraines commented May 14, 2018

waldenraines left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waldenraines May 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waldenraines commented May 14, 2018

waldenraines commented May 15, 2018

waldenraines May 14, 2018 •

edited

Loading