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

Improve renderSolution function if solution is empty #1909

Merged
merged 5 commits into from
Oct 10, 2023

Conversation

EMaksy
Copy link
Member

@EMaksy EMaksy commented Oct 6, 2023

Description

A quick fix to ensure that SaptuneDetails is rendered properly if solutions are empty
If solution is empty then applied and enabled solution will render '-'

Demo

image
image

How was this tested?

Added automated test

@EMaksy EMaksy added bug Something isn't working enhancement New feature or request labels Oct 6, 2023
@EMaksy EMaksy requested a review from arbulu89 October 6, 2023 12:24
@EMaksy EMaksy self-assigned this Oct 6, 2023
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

The solution is good.
Let's add a test and go for it

@EMaksy EMaksy force-pushed the saptune_details_view_render_solution_fix branch 2 times, most recently from 15b0440 to aac5c6f Compare October 9, 2023 11:24
@EMaksy EMaksy marked this pull request as ready for review October 9, 2023 11:24
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

Looking good.
I would create a more complete test to check all the "empty" value visualizations.
And update the test name accordingly

@EMaksy EMaksy force-pushed the saptune_details_view_render_solution_fix branch 2 times, most recently from 3bf9c1e to 44f15af Compare October 9, 2023 15:50
@@ -110,4 +110,54 @@ describe('SaptuneDetails', () => {
staging.solutions_ids
);
});

it.each([
{ solution: null },
Copy link
Contributor

Choose a reason for hiding this comment

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

why all of this strange things?
Except the null option the other 2 are not real cases. They won't happen, so we don't need to test them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough, the other cases are irrelevant, i just thought some extra error testing won't harm :D

enabledSolution={solution}
hostname={hostname}
hostID={hostID}
configuredVersion={configuredVersion}
Copy link
Contributor

Choose a reason for hiding this comment

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

Send enabledNotes and appliedNotes as empty lists and check we display them as "empty"

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah sure :D

@EMaksy EMaksy force-pushed the saptune_details_view_render_solution_fix branch 2 times, most recently from 7d8c54a to 560c5b9 Compare October 10, 2023 13:38
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

Noice!

@EMaksy EMaksy merged commit e1b1ef5 into main Oct 10, 2023
@EMaksy EMaksy deleted the saptune_details_view_render_solution_fix branch October 10, 2023 14: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 enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

2 participants