Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #23481 - Host Collection Action Modal Links #7353

Merged
merged 1 commit into from Jun 7, 2018

Conversation

sjha4
Copy link
Member

@sjha4 sjha4 commented May 2, 2018

Description:

The Host Collection Modals have links which do not work.


Steps to reproduce:

Go to Hosts> Host Collections > Any record > Click on Errata Installation
In the table in the modal, click on any link, it leads to an error in the console.

On the other modal links (eg: Host Collections, Subscriptions etc), even though the page redirects on a link click, the modal doesn't close.


Suggested solution:

  1. Open a modal to show details for Errata Installation Links (As was the case before):

modalactions

  1. Redirect to link on other modals.

@theforeman-bot
Copy link

Issues: #23481

@sjha4
Copy link
Member Author

sjha4 commented May 2, 2018

@Rohoover: Could you take a quick look at this?

  1. The plan is to open a new modal on top of existing modal with a close/Ok which takes users to the original modal.
  2. An alternative could be to redirect users to the clicked link.

@sjha4 sjha4 changed the title Fixes #23481 - Host Collection Action Modal Links [WIP] Fixes #23481 - Host Collection Action Modal Links May 2, 2018
Copy link

@Rohoover Rohoover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway we could put in a back button? This would prevent stacking modals.

@sjha4
Copy link
Member Author

sjha4 commented May 14, 2018

host collection_ test1

@Rohoover : Sorry about the messed up graphics. Some weird gif conversion issue.
The video shows both cases:

  1. Errata Id click which replaces the modal but needs a refresh of the previous modal on close (or "back").
  2. Host Id click which opens a second modal but doesn't need refresh of the parent modal on close.

Let me try a few alternative approaches around preventing stacking modals and I will get back to you..Thanks!

Copy link

@Rohoover Rohoover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sjha4

I prefer the modals not stack, that content be loaded into the same modal with a back button to go to the original modal.

@sjha4 sjha4 force-pushed the issue23481 branch 3 times, most recently from 6e5b72b to 27fda1b Compare May 15, 2018 15:10
@sjha4 sjha4 changed the title [WIP] Fixes #23481 - Host Collection Action Modal Links Fixes #23481 - Host Collection Action Modal Links May 15, 2018
@sjha4
Copy link
Member Author

sjha4 commented May 15, 2018

Updates: The Errata Modal should behave as requested now.
For the other modals, replicating the behavior from before of redirecting on link clicks.

@beav
Copy link
Contributor

beav commented May 18, 2018

[test prprocessor]

Copy link
Member

@akofink akofink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this is the 'right' way to go about fixing the links from a Javascript perspective. What is the transitionTo() function? Why were we using it before and why should we stop using it?

Even though I mention linking directly without modals, I ultimately think we should try to keep it working as it has been (with modals) if we can because it should decrease the code changes.

@@ -105,6 +105,103 @@ <h4 data-block="modal-header">Content Host Errata Management</h4>
</table>
</div>
</div>

<div name="errataDetails" ng-show="showErrata">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unclear why this is a better solution than simply linking to the erratum details page. If we really want to use modals, at least we could use a partial to reduce the risk of introducing bugs if something (i.e. the API) changes in the future. From a development point of view, it's just not DRY. Also, the erratum details page lists the repositories and content hosts affected as well, which I think (the repos) is useful information not currently provided by these modals.

I can see an argument for the modal in that it decreases context switching for our users. Maybe they just want to apply any applicable errata but want to just 'peek' at a few errata to see which packages are provided, etc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akofink : So this modal logic with link redirects had these 2 pages Errata Content host and Errata Details which become dead code following this change.

These 2 pages worked fine till 6.2 but once we moved to modals in 6.3, the redirects stopped working.

Do you suggest turning these pages into modals or a simple redirect to the regular errata detail page etc..(We still have some context switching involved with links on the other modals. I don't know why errata was treated differently in the first place)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... I guess we should fix it to work as was intended in 3.4 (6.3) with the modals? It's kind of a UX question, so maybe @Rohoover has some input. We should definitely still support all the workflows we used to support in 3.0 (6.2) before we changed over to modals.

@@ -80,7 +80,7 @@ <h4 data-block="modal-header" translate>
<tr bst-table-row ng-repeat-start="(name, subscriptions) in groupedSubscriptions">
<td class="row-select"></td>
<td colspan="8">
<a ui-sref="subscription.info({subscriptionId: subscription.id})" class="confined-text">
<a ng-click="ok();" ui-sref="subscription.info({subscriptionId: subscription.id})" class="confined-text">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This confirmation modal doesn't lock out the yes/no buttons after the first click, so I can easily get 500 errors from the server by clicking yes/no multiple times before I'm redirected to the task progress.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably should be made into a separate issue rather than being fixed in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will go ahead open an issue around the same.

The transitionTo() method was used to redirect to the errata details and content hosts page specific to the errata action on host collections.
I am trying not to use those page as redirect links but content inside the existing modal seems more user friendly instead of the redirection.

Copy link
Member

@akofink akofink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay. I wanted to spin up a 6.2 box to see how it used to work. I think the way you've implemented is 'correct', meaning that it seems to work very similar to the way it used to work in 6.2, just with modals! :)

If we're no longer using those two views, let's go ahead and delete them in this PR.

@sjha4
Copy link
Member Author

sjha4 commented Jun 1, 2018

I will go ahead and delete the 2 pages and open an issue around the yes/no button disabling. Thanks @akofink !

Copy link
Member

@akofink akofink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! ACK!

@akofink akofink merged commit 0ffd750 into Katello:master Jun 7, 2018
@sjha4 sjha4 deleted the issue23481 branch June 7, 2018 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants