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

Fix flickering in Available Software Updates panel #2789

Merged
merged 2 commits into from
Jul 18, 2024
Merged

Fix flickering in Available Software Updates panel #2789

merged 2 commits into from
Jul 18, 2024

Conversation

jamie-suse
Copy link
Contributor

@jamie-suse jamie-suse commented Jul 16, 2024

Description

This PR fixes an issue in the UI where the Available Software Updates panel was rendering two different loading states at two different times. The result of this was that the users were seeing a flicker as the panel (briefly) moved into a loading state a second time. This created a confusing user experience.

See the image below for the typical sequence of Redux events that would happen during rendering:

software_updates_loading

Note how the panel enters a loading state while retrieving the software updates, the response returns and the software updates are saved. Much later however the network call to retrieve the SUSE Manager settings begins, and sets the second loading state. The settings are then returned, and the panel is set into the display state again. This is a confusing user experience.

This was fixed by removing the superfluous call to retrieve the SUSE Manager settings, as they are not actually relevant to the Host Details page, nor the Available Software Updates panel. Since we know that if we can retrieve the software updates, then obviously the settings are configured correctly. To emphasize; we don't care about the specific SUSE Manager settings, just if we are authorized to access SUSE Manager with our settings.

We differentiate between being unauthorized vs some other network or server error by returning a 422 status code and corresponding error message "SUSE Manager authentication error." in the response from Trento's SUSE Manager Controller. The frontend can then know that the SUSE Manager settings are not configured correctly and displays the SUSE Manager is not configured state to the user.

How was this tested?

Units tests updated.

Did you update the documentation?

No changes are required for the documentation.

@jamie-suse jamie-suse added bug Something isn't working javascript Pull requests that update Javascript code elixir Pull requests that update Elixir code labels Jul 16, 2024
@jamie-suse jamie-suse self-assigned this Jul 16, 2024
@jamie-suse jamie-suse added the ux label Jul 16, 2024
@jamie-suse jamie-suse changed the title Fix issue with flickering in Available Software Updates panel Fix flickering in Available Software Updates panel Jul 17, 2024
@jamie-suse jamie-suse marked this pull request as ready for review July 17, 2024 11:43
Copy link
Contributor

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

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

This is terrific! Thanks for fixing all of this, this is a huge contribution 🔥

Comment on lines 20 to 26
def call(conn, {:error, :suma_authentication_error}) do
conn
|> put_status(:forbidden)
|> put_view(ErrorView)
|> render(:"403", reason: "SUSE Manager authentication error.")
end

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we now rely on 403 for our own permissions system it's better to switch this to 404 or 422, no big deal

({ detail }) => detail === 'SUSE Manager authentication error.'
);

if (errorCode === 422 && suma_unauthorized) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I check the status code here for completeness, though technically we may not need to.

@jamie-suse jamie-suse merged commit cecd0f3 into main Jul 18, 2024
26 checks passed
@jamie-suse jamie-suse deleted the flicker branch July 18, 2024 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working elixir Pull requests that update Elixir code javascript Pull requests that update Javascript code ux
Development

Successfully merging this pull request may close these issues.

2 participants