Fixes #22734 - Add ability to edit RH Subscription entitlements #7317
Conversation
|
Issues: #22734 |
|
|
||
| return ( | ||
| <td className="editable"> | ||
| <div |
There was a problem hiding this comment.
Visible, non-interactive elements with click handlers must have at least one keyboard listener jsx-a11y/click-events-have-key-events
Static HTML elements with event handlers require a role jsx-a11y/no-static-element-interactions
| this.state = { | ||
| updatedQuantity: {}, | ||
| editing: false, | ||
| rows: props.subscriptions.results, |
There was a problem hiding this comment.
'subscriptions.results' is missing in props validation react/prop-types
There was a problem hiding this comment.
Initial review is mainly nitpicks. I will look at the code in more depth for my next review.
I am, however, getting the following webpack compilation errors which prevent me from testing:
02:09:51 webpack.1 | [0] ./node_modules/css-loader?sourceMap!./node_modules/sass-loader/lib/loader.js?sourceMap!../katello/webpack/scenes/Subscriptions/Subscriptions.scss 310 bytes {0} [built] [failed] [1 error]
02:09:51 webpack.1 |
02:09:51 webpack.1 | ERROR in ./node_modules/css-loader?sourceMap!./node_modules/sass-loader/lib/loader.js?sourceMap!../katello/webpack/scenes/Subscriptions/Subscriptions.scss
02:09:51 webpack.1 | Module build failed:
02:09:51 webpack.1 | @import '~patternfly-react/dist/sass/inline-edit';
02:09:51 webpack.1 | ^
02:09:51 webpack.1 | File to import not found or unreadable: ~patternfly-react/dist/sass/inline-edit.
02:09:51 webpack.1 | Parent style sheet: stdin
02:09:51 webpack.1 | in /home/vagrant/katello/webpack/scenes/Subscriptions/Subscriptions.scss (line 2, column 1)
02:09:51 webpack.1 | Child extract-text-webpack-plugin node_modules/extract-text-webpack-plugin/dist node_modules/css-loader/index.js?sourceMap!node_modules/sass-loader/lib/loader.js?sourceMap!../katello/webpack/scenes/Subscriptions/Subscriptions.scss:
02:09:51 webpack.1 | [0] ./node_modules/css-loader?sourceMap!./node_modules/sass-loader/lib/loader.js?sourceMap!../katello/webpack/scenes/Subscriptions/Subscriptions.scss 310 bytes {0} [built] [failed] [1 error]
02:09:51 webpack.1 |
02:09:51 webpack.1 | ERROR in ./node_modules/css-loader?sourceMap!./node_modules/sass-loader/lib/loader.js?sourceMap!../katello/webpack/scenes/Subscriptions/Subscriptions.scss
02:09:51 webpack.1 | Module build failed:
02:09:51 webpack.1 | @import '~patternfly-react/dist/sass/inline-edit';
02:09:51 webpack.1 | ^
02:09:51 webpack.1 | File to import not found or unreadable: ~patternfly-react/dist/sass/inline-edit.
02:09:51 webpack.1 | Parent style sheet: stdin
02:09:51 webpack.1 | in /home/vagrant/katello/webpack/scenes/Subscriptions/Subscriptions.scss (line 2, column 1)
02:09:51 webpack.1 | webpack: Failed to compile.
.eslintrc
Outdated
| @@ -15,6 +15,7 @@ | |||
| "react" | |||
| ], | |||
| "rules": { | |||
| "no-console": "off", | |||
There was a problem hiding this comment.
Why do we want to allow console statements?
There was a problem hiding this comment.
I often use console logs for development. I can put it back if you prefer to keep it disabled.
There was a problem hiding this comment.
I do too but I prefer to keep it disabled to avoid accidentally merging the many console.log("WTF WHY ISN'T THIS WORKING?!?!?!?"); statements I often put into my code 😄
There was a problem hiding this comment.
+1 to keeping eslint checking for console statements, I also tend to leave them in my code and would like eslint to check for this
| .editable { | ||
| background: none; | ||
| } | ||
| } |
There was a problem hiding this comment.
We probably want to put this into overrides.scss.
| <ManageManifestModal showModal={this.state.manifestModalOpen} onClose={onModalClose} /> | ||
|
|
||
| { this.renderSubscriptionTable() } | ||
| <SubscriptionsTable |
There was a problem hiding this comment.
Nice change here, thanks!
| }; | ||
|
|
||
| const buildTableRows = (subscriptions, updatedQuantity) => | ||
| subscriptions.map((x) => { |
There was a problem hiding this comment.
Thoughts on changing x to row or something else descriptive?
| /> | ||
| {__('Max')} | ||
| {' '} | ||
| {additionalData.rowData.available} |
There was a problem hiding this comment.
Nitpick: what about {[__('Max'), additionalData.rowData.available].join(' ')}?
Just realized that I forgot to install patternfly-react from patternfly/patternfly-react#240. After doing so I now get: But I'm not sure that I'm doing this correctly, I had to checkout your branch within |
I have a checkout outside of |
| {[__('Max'), additionalData.rowData.available].join(' ')} | ||
| </td> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Missing trailing comma comma-dangle
If I symlink to In fact, I get this error: And if I symlink to |
|
@waldenraines my bad, you're right. I removed just the |
I did this (although it should be the exact same as symlinking the entire And just to prove that |
My mistake, I symlinked to |
There was a problem hiding this comment.
I had a chance to test this out and it works for the most part, nice 👍 Here are some issues I found:
- There are no errors if you set the number of entitlements to below the max or to an invalid value ("asfhgsdh", for example)
- I'm seeing -1 entitlements for custom products. I assume we should so "Unlimited" in this case, but will need to confirm
- Sometimes upon editing an entitlement amount I am seeing it revert to the previous value. It's usually the first time I edit a value that I am seeing this.
- I'm seeing
emptyStateupon initial load even though I have subscription results in the API response. Once I go to the Add Subscriptions page and then back it loads correctly. - I get a JS error when going to the next page after editing a subscription:
SubscriptionsTable.js:88 Uncaught TypeError: Cannot read property 'quantity' of undefined
at Object.hasChanged (SubscriptionsTable.js:88)
at renderEdit (SubscriptionsTableSchema.js:35)
at InlineEdit (InlineEdit.js:21)
at mountIndeterminateComponent (react-dom.development.js:8032)
at beginWork (react-dom.development.js:8221)
at performUnitOfWork (react-dom.development.js:10224)
at workLoop (react-dom.development.js:10288)
at HTMLUnknownElement.callCallback (react-dom.development.js:542)
at Object.invokeGuardedCallbackDev (react-dom.development.js:581)
at invokeGuardedCallback (react-dom.development.js:438)
| {[ | ||
| __('Max'), | ||
| additionalData.rowData.available, | ||
| ].join(' ')} |
|
Updated:
We might need to update Known issues:
Additional comments: @waldenraines I fixed the validations. Reverting to the previous values right after confirmation should be fixed once we introduce the overlay progressbar for running tasks. It's expected behavior at the moment. |
tstrachota
changed the title
There was a problem hiding this comment.
Inline validation occurs below the form field:
http://www.patternfly.org/pattern-library/forms-and-controls/inline-edit/#design
You are correct that the tooltip for inline editing errors would be out of standard. Technically the "Max 500" being red is correct.
We will need to do that, and have the rpm built as well.
Works for me.
👍 I'm copying the list from my previous comment here. Will check off the items that I can no longer reproduce after I'm unblocked in testing by this first issue.
|
| {paginationComponent} | ||
| </div> | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| export default Table; |
There was a problem hiding this comment.
Do you need to export default Table if you are doing export class Table above?
There was a problem hiding this comment.
Not necessarily. I just tend to always add a default export if there's some "main" component in the module.
|
Getting this error while viewing the page |
| hover | ||
| columns={columns} | ||
| {...otherProps} | ||
| > |
There was a problem hiding this comment.
Is there a way to not repeat PfTable.PfProvider in this if/else? Asking as I will have to add to it and doing so will duplicate more code
|
@johnpmitsch it now depends on theforeman/foreman#5514 @waldenraines could you describe when you see the |
I see that when attempting to edit any subscription. I have not filed an issue for this but we can if you would prefer to address this issue outside of this PR. |
| title={__('Editing Entitlements')} | ||
| dangerouslySetInnerHTML={{ | ||
| __html: sprintf( | ||
| __("You're making changes to %(entitlementCount)s entitlements"), |
There was a problem hiding this comment.
nitpick make this entitlement(s)
|
@tstrachota This worked well and code looks good. 👍 There are a few small comments/questions that still need to be addressed. |
|
I can submit a value larger than the max allowed (even if the text box is red and displaying a validation error). Shouldn't the submit check be disabled when the quantity is higher than the max? |
|
error is fine, as long as it's handled before the task is kicked off |
| @@ -0,0 +1,40 @@ | |||
| import React from 'react'; | |||
| import { shallow, mount } from 'enzyme'; | |||
There was a problem hiding this comment.
'mount' is defined but never used no-unused-vars
|
Updated:
Following should be fixed as a separate issues (and I still need to create redmine tickets for that):
Notes after rebase: |
There was a problem hiding this comment.
Really nice work overall!
A few issues I'm seeing:
- I still get the empty state sometimes when I first visit the page.
- The first time I edit any entitlements I get a spinner that spins forever until I cancel the edit and try it again. The 2nd time after the page is loaded it works.
- Hitting tab within an entitlement input field doesn't tab me to the next field.
- For the error handling do we want to show the inline alert or should we create toast notifications for these errors instead? @Rohoover any input on this?
- I don't see the validation error dialog or message when validation errors are triggered. Instead it just submits the request.
I am fine if you want to fix these after the fact or within this PR, it's up to you.
| <div | ||
| onClick={() => inlineEditController.onActivate(additionalData)} | ||
| onKeyPress={(e) => { | ||
| if (e.key === 'Enter') { |
There was a problem hiding this comment.
Is this a safe way to do this? I thought you had to use onKeyDown() and e.charCode or e.keyCode.
|
|
||
| export const SUBSCRIPTIONS_QUANTITIES_REQUEST = 'SUBSCRIPTIONS_QUANTITIES_REQUEST'; | ||
| export const SUBSCRIPTIONS_QUANTITIES_SUCCESS = 'SUBSCRIPTIONS_QUANTITIES_SUCCESS'; | ||
| export const SUBSCRIPTIONS_QUANTITIES_FAILURE = 'SUBSCRIPTIONS_QUANTITIES_FAILURE'; |
There was a problem hiding this comment.
Should the task generated by these actions be added to the list of blocking task types below?
There was a problem hiding this comment.
I don't think so. The action runs on background so there's no need to block the UI for users. If the start editing prior to successful fetch a spinner is displayed.
There was a problem hiding this comment.
Sorry, I meant to put this comment under the update actions. I mean that we need to add Actions::Katello::UpstreamSubscriptions::UpdateEntitlement to the list of BLOCKING_FOREMAN_TASK_TYPES below so that the buttons are disabled correctly when this task is in progress.
There was a problem hiding this comment.
That makes sense. I didn't understand it first. Updated.
| @@ -30,55 +27,10 @@ class SubscriptionsPage extends Component { | |||
| type: 'all', | |||
| active_only: true, | |||
| action_types: BLOCKING_FOREMAN_TASK_TYPES, | |||
| }, 10000); | |||
| }, 99999999999); | |||
There was a problem hiding this comment.
Intentional? See also http://projects.theforeman.org/issues/23475 which deals with coming up with a smarter way to poll for these tasks.
|
Updated
I'll deal with it separately. Since I couldn't reproduce it, would you mind reporting the issues? I guess that you'll be able to provide closer details like snapshots of the redux state and better description than me.
This one is difficult to fix. Reactabular calls
Fixed. There was an issue in the validation methods. I added some tests for it. |
There was a problem hiding this comment.
@waldenraines I'm not sure the inline is needed when there are no subscriptions to view. That's not really an error, it's just an empty state.
For errors on the form, I'm debating whether we're good with just for the form handling since the toast disappears and inline messages tend to cause complaints of content shifting.
Now I can't save any field, I always get the validation error dialog even if there is no validation error. |
Oops, I forgot to exclude custom subscriptions from the validation. I'm sorry for that. Try now, please. |
These issues went away after I blew away |
|
Even though this was merged, for this modal, I think we should say, "Do you want to cancel without saving your changes" to make it extra clear that you are cancelling the form. "continue" makes me second guess that its going to submit. |
|
I'm just realizing there is a "Cancel" button as well, maybe have "Yes" and "No" for the buttons? @Rohoover how does this sound to you? |
Contains also Walden's 748a501 which will be removed eventually.
It requires patternfly/patternfly-react#240 (review) for the inline edit capabilities.
TODO: