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 #37313 - Change Content Source form improvements #10955

Merged

Conversation

jeremylenz
Copy link
Member

@jeremylenz jeremylenz commented Apr 8, 2024

What are the changes introduced in this pull request?

Several improvements to the Change Content Source page:

  1. Add a url parameter, initialContentSourceId. When this is specified, the dropdown's initial value is set. In addition, if there is only one host to update, and initialContentSourceId is passed, the form assumes that you are not really changing the host's content source, and instead only the host content view environment is changing. In such a case, the UI is improved accordingly (see the following items).
  2. Add a banner for the situation above, letting you know that the content source is not changing.
  3. In the situation above - Instead of "Run job invocation" / "Update manually" buttons, there is only a single "Save" button.
  4. The 'Change content source' link from the new host details page now specifies initialContentSourceId.
  5. The 'Change content source' link from the Edit Hosts page now specifies initialContentSourceId.

Considerations taken when implementing this change?

Currently the form can't check the host's real content source ID against the selected one because the API response it's using for the host list is quite "thin," and only contains host ID and name. So I came up with the logic above as a sort of compromise. I think that most users will not manually compose the URL with URL params, and so a situation where initialContentSourceId doesn't match the single host's real content source ID is unlikely.

What are the testing steps for this pull request?

test all flows and make sure everything still works

  • link from Edit Host
  • link from host details
  • Bulk select from new All Hosts
  • bulk select from old All Hosts
  • single host
  • multi-host

@jeremylenz jeremylenz force-pushed the 37313-cv-lce-select-content-source-page branch from 084406b to f8c49f3 Compare April 9, 2024 13:38
@jeremylenz jeremylenz marked this pull request as ready for review April 9, 2024 17:37
@jeremylenz jeremylenz force-pushed the 37313-cv-lce-select-content-source-page branch from f8c49f3 to b65428e Compare April 9, 2024 17:39
Copy link
Member

@ianballou ianballou left a comment

Choose a reason for hiding this comment

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

The very first thing I tested when figuring out what was changed here was the Host Details -> Change Content Source page.
When I realized you had to go through the Host Edit page, I wondered why we don't have the same fancy new banner on the regular change content source page coming from Host Details.
A user would still benefit from the simplicity of just clicking "save".

Edit: I see now that what I wrote above is the case. Looks like my browser cache was working against me again,

Copy link
Member

@ianballou ianballou left a comment

Choose a reason for hiding this comment

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

Would it make sense to redirect back to the host details page after changing the content source? The bread crumb leads back to host details rather than the host edit page, so that sorta supports the idea.

@ianballou
Copy link
Member

ianballou commented Apr 19, 2024

The Redmine mentions that it's confusing to have to select "update hosts manually" to only update CV & LCE. Seems this is fixed for single hosts, but not for bulk. Is there anything that could be done to make the situation less confusing for bulk changing LCE + CV via the Change Content Source form?

This thinking is stemming from the fact that the UI workflow will now have two different flows for single and bulk hosts. Single host content source change gains ease of understanding with "Save", but we also lose some workflow consistency.

@jeremylenz
Copy link
Member Author

jeremylenz commented Apr 19, 2024

Seems this is fixed for single hosts, but not for bulk. Is there anything that could be done to make the situation less confusing for bulk changing LCE + CV via the Change Content Source form?

I don't think this is a concern, because the user won't end up at this screen with the "multiple hosts + only change CV/LCE" combo. That's currently covered by content host bulk actions, and will soon also be covered by #10959.

Also I don't think we should be using this for that combo anyway, because the content source dropdown always sets the content source, which is not the same as changing only the CV/LCE.

@jeremylenz
Copy link
Member Author

Would it make sense to redirect back to the host details page after changing the content source? The bread crumb leads back to host details rather than the host edit page, so that sorta supports the idea.

Yeah probably! I'll have to see how much effort that would take. Also it's a bit complicated because where do we redirect to if it's multiple hosts?

@ianballou
Copy link
Member

Yeah probably! I'll have to see how much effort that would take. Also it's a bit complicated because where do we redirect to if it's multiple hosts?

What would you think about redirecting back to the all hosts page at that point? I also dunno if there's an easy way to say "go back to the page I was at before if there is one". Can users get to that page from any other place in the UI besides the host listing page?

@jeremylenz
Copy link
Member Author

Can users get to that page from any other place in the UI besides the host listing page?

There's 3 places afaik:
a) new host details
b) old host list
c) new host list

@jeremylenz
Copy link
Member Author

jeremylenz commented Apr 22, 2024

We could add a redirect, but only if you have 1 host to update and pass the new url param. Would that work for you?

@ianballou
Copy link
Member

We could add a redirect, but only if you have 1 host to update and pass the new url param. Would that work for you?

This works for me 👍

@jeremylenz jeremylenz force-pushed the 37313-cv-lce-select-content-source-page branch 2 times, most recently from e4fbd8a to 820e951 Compare April 23, 2024 19:33
@jeremylenz
Copy link
Member Author

@ianballou Added redirect back to the page you came from, for

  • host Edit
  • host details.

Should be good to go now..

Copy link
Member

@ianballou ianballou 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 how easily this could be fixed, but I noticed a funny workflow:

  1. Change content source via the form -- click 'Update hosts manually.
  2. Reload the page.
  3. See that initialContentSourceId=1 is still in the URL, so now the UI says that the content source isn't changing even if it will.

I wanted to change the content source again, so I reloaded the page instead of closing it and reopening it. It's a bit of a weird case, but I wanted to see opinions about it.

@jeremylenz
Copy link
Member Author

  • Change content source via the form -- click 'Update hosts manually.
  • Reload the page.
  • See that initialContentSourceId=1 is still in the URL, so now the UI says that the content source isn't changing even if it will.

The reason we rely on a URL param and "trust" that that's your host's existing content source ID is that the API request this page makes for hosts is not the traditional one-- rather, it's the "thin" one that only shows id and name. To fix this edge case we would have to change that. You could argue that isn't really worth it and would also take away the speed benefit of using the thin endpoint. If we do get a customer or user complaint about it down the road, though, we know how to fix it :)

@ianballou
Copy link
Member

  • Change content source via the form -- click 'Update hosts manually.
  • Reload the page.
  • See that initialContentSourceId=1 is still in the URL, so now the UI says that the content source isn't changing even if it will.

The reason we rely on a URL param and "trust" that that's your host's existing content source ID is that the API request this page makes for hosts is not the traditional one-- rather, it's the "thin" one that only shows id and name. To fix this edge case we would have to change that. You could argue that isn't really worth it and would also take away the speed benefit of using the thin endpoint. If we do get a customer or user complaint about it down the road, though, we know how to fix it :)

Having to fetch all host info is what I was worried about being the solution -- I agree that it doesn't seem worth the trouble. I can't think of a way around that at the moment either.

@jeremylenz jeremylenz force-pushed the 37313-cv-lce-select-content-source-page branch from 820e951 to 3308365 Compare April 24, 2024 14:22
@jeremylenz
Copy link
Member Author

Updated ouiaIds and squashed

@ianballou
Copy link
Member

Are the React failures related to the ouiaId change?

@jeremylenz jeremylenz force-pushed the 37313-cv-lce-select-content-source-page branch from 3308365 to c4ac727 Compare April 24, 2024 14:42
@jeremylenz
Copy link
Member Author

Are the React failures related to the ouiaId change?

nah, I think I just needed to rebase after the merge of #10976, let's see..

@jeremylenz
Copy link
Member Author

okay, that wasn't it. But hopefully #10978 should fix it on master, in which case I'll rebase here.

@jeremylenz jeremylenz force-pushed the 37313-cv-lce-select-content-source-page branch from c4ac727 to 287bf14 Compare April 24, 2024 17:51
@jeremylenz jeremylenz merged commit 737a6f2 into Katello:master Apr 24, 2024
21 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants