Fixes #22730: add manage manifest modal to subscriptions page. #7305

waldenraines · 2018-04-11T22:11:46Z

http://projects.theforeman.org/issues/22730

TODO:

Tests
Update manifest filename/etc on successful upload/delete
Modal opening up each time get task returns
Add progress bar above table for tasks that are running if the modal is closed
Add a modal on top of the modal to confirm deletion of manifest

Questions for UX:

How to handle delete manifest confirmation in modal
Show a progress bar when modal is exited? Right now if you refresh, upload, or delete and then navigate away and then back we don't show you the progress of your task.

theforeman-bot · 2018-04-11T22:11:52Z

Issues: #22730

waldenraines · 2018-04-11T22:14:47Z

Questions for UX:

@Rohoover, see above please.

Rohoover

So, we have confirmation of a modal on modal use. This can be adapted for delete confirmation. Let me know what you think about this or if you want to see a mockup first.

As for a progress bar, here are a couple thoughts:
We can disable all table actions (except for Manage Manifest) and continue to show the progress bar in the manifest modal. We could also - if technically feasible - place a progress bar on the content area of the table. Perhaps the content is faded back, or replaced completely. I can mock this up too if think this is worth pursuing.

waldenraines · 2018-04-12T19:00:45Z

So, we have confirmation of a modal on modal use. This can be adapted for delete confirmation. Let me know what you think about this or if you want to see a mockup first.

Do you have an screenshot of this from another application? That would suffice for me, I don't need a mockup of this particular page.

We can disable all table actions (except for Manage Manifest) and continue to show the progress bar in the manifest modal. We could also - if technically feasible - place a progress bar on the content area of the table. Perhaps the content is faded back, or replaced completely. I can mock this up too if think this is worth pursuing.

Well currently there is a "bug" that the manage manifest modal pops open every 3 seconds if you close it when the progress bar is updated. This could be a "feature" by just ensuring the modal is open when a task is ongoing and then just show the progress bar there. Just a thought.

Rohoover · 2018-04-12T19:06:28Z

@waldenraines

Here is the modal on modal -- mind you, it's not a delete confirmation.

Hmmmm, not a bad idea. Throwing this thought out there to think about... will it be annoying? Is there anything behind the modal that they would want or could interact with that we would be preventing them from doing so, or annoying them with a modal popping up every 3 seconds? If they close the modal, what's telling them that something is in progress? I guess that second question is what makes me think we need some other mechanism.

waldenraines · 2018-04-12T19:25:10Z

Hmmmm, not a bad idea. Throwing this thought out there to think about... will it be annoying? Is there anything behind the modal that they would want or could interact with that we would be preventing them from doing so, or annoying them with a modal popping up every 3 seconds? If they close the modal, what's telling them that something is in progress? I guess that second question is what makes me think we need some other mechanism.

Yeah, true, I wasn't sold on it even when I proposed it. I think a progress bar on the subscriptions page itself would be a better way to go. Do you have a strong preference to put it in the table versus above it?

Rohoover

Let's put it above it, much like how the modal has a dimmed background, so users don't think the content was all blown away.

Rohoover · 2018-04-12T19:40:44Z

Something like this:

I came up with this pretty quickly so I am flexible if you have another idea.

waldenraines · 2018-04-12T20:02:36Z

Something like this:

Oh, interesting, when I said above I meant in the content area above the table but I like this better.

waldenraines · 2018-04-13T13:22:11Z

Update manifest filename/etc on successful upload/delete
Modal opening up each time get task returns
Add progress bar above table for tasks that are running if the modal is closed
Add a modal on top of the modal to confirm deletion of manifest

Does anyone care if I complete these items as follow on PRs?

waldenraines · 2018-04-15T22:14:07Z

Tests added, would like to handle the rest of the todo items as separate PRs. Thoughts?

waldenraines · 2018-04-16T12:30:37Z

Update manifest filename/etc on successful upload/delete

http://projects.theforeman.org/issues/23283

Modal opening up each time get task returns

http://projects.theforeman.org/issues/23284

Add progress bar above table for tasks that are running if the modal is closed

http://projects.theforeman.org/issues/23285

Add a modal on top of the modal to confirm deletion of manifest

http://projects.theforeman.org/issues/23286

waldenraines · 2018-04-16T12:35:20Z

webpack/scenes/Tasks/TaskActions.js

+  TASK_BULK_SEARCH_FAILURE,
+} from './TaskConstants';
+
+export const bulkSearch = (extendedParams = {}) => (dispatch) => {


These task related actions, reducers, and constants should probably be move to foreman (or at least the move_to_foreman directory 😄)

tstrachota

Nice @waldenraines !

I went through the code and did some testing. It works mostly fine. I added some questions and nitpicks inline. Given that this is still in the labs section, I agree it's fine to fix all of them as separate PRs as long as we track it somewhere. Maybe just saving the org to state could be fixed now.

Btw I noticed TypeError: deburr(...).replace is not a function after both delete and upload manifest. Not sure what causes that.

tstrachota · 2018-04-16T15:57:58Z

webpack/services/api/index.js

+  }
+}
+
+export const foremanTasksApi = new ForemanTasksApi();


tstrachota · 2018-04-16T16:32:44Z

webpack/scenes/Subscriptions/Manifest/ManageManifestModal.js

+
+    const emptyStateData = () => ({
+      header: __('There is no Manifest History to display.'),
+      description: __('Import a Manifest using the details tab.'),


In fact the tab is called "Manifest", not "details".

tstrachota · 2018-04-16T16:34:40Z

webpack/scenes/Subscriptions/Manifest/ManageManifestModal.js

+        url: 'http://redhat.com',
+      },
+      action: {
+        title: __('Upload Manifest'),


This button has no effect. Can we just hide it?

tstrachota · 2018-04-16T16:39:54Z

webpack/scenes/Organizations/OrganizationReducer.js

+
+    case GET_ORGANIZATION_SUCCESS:
+    case SAVE_ORGANIZATION_SUCCESS: {
+      return Immutable({ loading: false }, ...action.response);


I guess this should have been Immutable({ loading: false, ...action.response}). Currently the fetched org data doesn't get stored in the state.

tstrachota · 2018-04-16T16:44:14Z

webpack/scenes/Subscriptions/Manifest/ManageManifestModal.js

+      showModal: props.showModal,
+    };
+
+    this.hideModal = this.hideModal.bind(this);


Something like https://github.com/patternfly/patternfly-react/blob/master/src/common/helpers.js#L3-L7 would be handy. But no need to add it in this PR.

Oh, thanks, I didn't even know that existed!

tstrachota · 2018-04-16T16:47:34Z

webpack/scenes/Subscriptions/Manifest/ManageManifestModal.js

+
+                <FormGroup>
+                  <ControlLabel className="col-sm-3 control-label" htmlFor="usmaFile">
+                    {__('USMA')}


I think that we're missing an info tooltip here (what is USMA? :))

You're right, we should have a tooltip here that shows "Upstream Subscription Management Application".

tstrachota · 2018-04-16T16:53:54Z

webpack/scenes/Organizations/__tests__/organizations.fixtures.js

+  library_id: 1,
+});
+
+export const successState = Immutable({ loading: false }, ...requestSuccessResponse);


I was wondering why the tests don't fail:)
{ loading: false, ...requestSuccessResponse } here too

tstrachota · 2018-04-16T17:00:54Z

webpack/scenes/Subscriptions/SubscriptionsPage.js

  componentDidMount() {
    this.loadData();
  }

  loadData() {
+    this.props.pollBulkSearch({
+      search_id: 'pollBulkSearch',


Shouldn't we use some more specific search_id? activeManifestTasksSearch or something similar.
It should probably be a constant too.

tstrachota · 2018-04-16T17:11:08Z

webpack/scenes/Tasks/TaskReducer.js

+
+const initialState = Immutable({ loading: false, results: [] });
+
+export default (state = initialState, action) => {


Is this reducer and state used anywhere or are you planning to use it anywhere? Or does it help debugging?
I'm just wondering if it makes sense to store results of any search in the generic state node.

This can probably just be removed. I was using it at first but then changed my implementation. I will remove.

waldenraines · 2018-04-17T14:54:16Z

Maybe just saving the org to state could be fixed now.

I'd really prefer getting all of these large pieces in first and fixing these known issues after the fact if at all possible.

waldenraines · 2018-04-17T14:56:39Z

Btw I noticed TypeError: deburr(...).replace is not a function after both delete and upload manifest. Not sure what causes that.

http://projects.theforeman.org/issues/23302

tstrachota · 2018-04-17T15:02:18Z

I'd really prefer getting all of these large pieces in first and fixing these known issues after the fact if at all possible.

NP, it's a simple issue, we can do it after

waldenraines · 2018-04-17T15:29:22Z

@tstrachota updated. I believe I have fixed all of your comments.

tstrachota

👍 let's merge this

tstrachota · 2018-04-17T16:15:20Z

...once the tests are green, of course 😃

waldenraines · 2018-04-17T17:28:03Z

[test katello]

jturel · 2018-04-17T18:18:49Z

I'm fine with this being merged as-is, and I know you're aware of something the things I'm mentioning here, but here's my feedback:

shows No Manifest Imported even though I have one
upon refreshing manifest and closing the modal, I can not open the modal via Manage Manifest button again (disabled)
however the button became enabled again when refresh was complete (good)
messaging "Refresh Manifest Completed Successfully." should this also be shown as an ordinary notification, not just in the modal?
"USMA" - can we expand this acronym on the page?
cdn url is sending a PUT to the API on each keypress in the text field & looks like its not actually updating it on the backend
cdn URL does not show the current value

tstrachota · 2018-04-17T18:23:52Z

@jturel did you test Walden's latest commit? I was experiencing some of the issues you're describing but they got fixed with the latest update.

jturel · 2018-04-17T18:40:43Z

@tstrachota ah - nope. let me pull that down and take a look

waldenraines · 2018-04-17T19:02:30Z

[test katello]

waldenraines · 2018-04-17T19:18:36Z

shows No Manifest Imported even though I have one

Hmm, odd, was that right after you imported one?

upon refreshing manifest and closing the modal, I can not open the modal via Manage Manifest button again (disabled)
however the button became enabled again when refresh was complete (good)

I suppose we could look at not disabling the entire modal but just disabling the actions within the modal that cannot be done while an upload/delete/refresh is in progress.

messaging "Refresh Manifest Completed Successfully." should this also be shown as an ordinary notification, not just in the modal?

@Rohoover thoughts on that?

"USMA" - can we expand this acronym on the page?

There is now a tooltip that shows what it stands for.

cdn url is sending a PUT to the API on each keypress in the text field & looks like its not actually updating it on the backend

It should be but I have not yet made it grab the latest organization upon successful save.

cdn URL does not show the current value

See above.

waldenraines · 2018-04-17T21:04:10Z

[test katello]

waldenraines · 2018-04-18T01:44:16Z

[test katello]

waldenraines · 2018-04-18T03:21:58Z

[test katello]

jturel · 2018-04-18T04:07:09Z

Hmm, odd, was that right after you imported one?

Yes!

Extra note:

old manifest name shows after manifest delete (i did the delete right after a refresh if it matters) & i could still refresh (unexpected) which led to an (expected) error: "Upstream identity certificate not available

There is now a tooltip that shows what it stands for.

ok, that helps but the acronym just feels out of place to me.

jturel

Shaping up very nicely & left my thoughts. Thanks!

APJ

waldenraines · 2018-04-18T04:14:51Z

ok, that helps but the acronym just feels out of place to me.

That came from the mockups, @Rohoover thoughts?

waldenraines · 2018-04-18T04:16:23Z

old manifest name shows after manifest delete (i did the delete right after a refresh if it matters) & i could still refresh (unexpected) which led to an (expected) error: "Upstream identity certificate not available

Yeah, I think that's due to the lack of an organization refresh, I hope to fix that as part of http://projects.theforeman.org/issues/23283

Rohoover

@waldenraines

Yes on disabling buttons in modal.
Yes on an extra notification of completed refresh.

jlsherrill · 2018-04-19T19:37:28Z

[test katello]

waldenraines · 2018-04-19T20:05:52Z

[test katello]

http://projects.theforeman.org/issues/22730

waldenraines · 2018-04-19T21:51:34Z

[test katello]

theforeman-bot added Needs testing Not yet reviewed labels Apr 11, 2018

Rohoover reviewed Apr 12, 2018

View reviewed changes

waldenraines force-pushed the 22730 branch from ffae285 to ef7d43b Compare April 15, 2018 22:13

waldenraines changed the title ~~[WIP] Fixes #22730: add manage manifest modal to subscriptions page.~~ Fixes #22730: add manage manifest modal to subscriptions page. Apr 15, 2018

waldenraines force-pushed the 22730 branch from ef7d43b to bb28f9e Compare April 15, 2018 22:14

waldenraines commented Apr 16, 2018

View reviewed changes

tstrachota reviewed Apr 16, 2018

View reviewed changes

waldenraines force-pushed the 22730 branch from bb28f9e to 735f7a8 Compare April 17, 2018 15:28

tstrachota approved these changes Apr 17, 2018

View reviewed changes

jturel self-assigned this Apr 17, 2018

jturel approved these changes Apr 18, 2018

View reviewed changes

Rohoover reviewed Apr 18, 2018

View reviewed changes

Fixes #22730: add manage manifest modal to subscriptions page.

9b215da

http://projects.theforeman.org/issues/22730

waldenraines force-pushed the 22730 branch from 735f7a8 to 9b215da Compare April 19, 2018 20:37

waldenraines merged commit 2a5bde8 into Katello:master Apr 19, 2018

waldenraines deleted the 22730 branch April 19, 2018 22:50


		const initialState = Immutable({ loading: false, results: [] });

		export default (state = initialState, action) => {

Fixes #22730: add manage manifest modal to subscriptions page. #7305

Fixes #22730: add manage manifest modal to subscriptions page. #7305

Conversation

waldenraines commented Apr 11, 2018 • edited Loading

theforeman-bot commented Apr 11, 2018

waldenraines commented Apr 11, 2018

Rohoover left a comment

Choose a reason for hiding this comment

waldenraines commented Apr 12, 2018 • edited Loading

Rohoover commented Apr 12, 2018

waldenraines commented Apr 12, 2018 • edited Loading

Rohoover left a comment

Choose a reason for hiding this comment

Rohoover commented Apr 12, 2018

waldenraines commented Apr 12, 2018

waldenraines commented Apr 13, 2018

waldenraines commented Apr 15, 2018 • edited Loading

waldenraines commented Apr 16, 2018

waldenraines Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

tstrachota 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

waldenraines commented Apr 17, 2018

waldenraines commented Apr 17, 2018

tstrachota commented Apr 17, 2018

waldenraines commented Apr 17, 2018

tstrachota left a comment

Choose a reason for hiding this comment

tstrachota commented Apr 17, 2018

waldenraines commented Apr 17, 2018

jturel commented Apr 17, 2018

tstrachota commented Apr 17, 2018

jturel commented Apr 17, 2018

waldenraines commented Apr 17, 2018

waldenraines commented Apr 17, 2018

waldenraines commented Apr 17, 2018

waldenraines commented Apr 18, 2018

waldenraines commented Apr 18, 2018

jturel commented Apr 18, 2018

jturel left a comment

Choose a reason for hiding this comment

waldenraines commented Apr 18, 2018

waldenraines commented Apr 18, 2018

Rohoover left a comment

Choose a reason for hiding this comment

jlsherrill commented Apr 19, 2018

waldenraines commented Apr 19, 2018

waldenraines commented Apr 19, 2018

waldenraines commented Apr 11, 2018 •

edited

Loading

waldenraines commented Apr 12, 2018 •

edited

Loading

waldenraines commented Apr 12, 2018 •

edited

Loading

waldenraines commented Apr 15, 2018 •

edited

Loading

waldenraines Apr 16, 2018 •

edited

Loading