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(THEEDGE-3703): On immutable tab we can't see the add system option #2102

Merged
merged 21 commits into from
Nov 27, 2023

Conversation

acosferreira
Copy link
Contributor

@acosferreira acosferreira commented Nov 22, 2023

Fixes https://issues.redhat.com/browse/THEEDGE-3703.

As part of this ticket, iwe moved the list of immutable from federation module to insights code, adding the update actions inline and bulk selection based on image_set values.
We also match the mockup from #3581.
Now he have add system, update bulk selection, update inline, add/remove from group and tab list.

THEEDGE-3705.webm

@codecov-commenter
Copy link

codecov-commenter commented Nov 22, 2023

Codecov Report

Attention: 134 lines in your changes are missing coverage. Please review.

Comparison is base (f4a9b0d) 59.22% compared to head (c6a8adf) 57.96%.

Files Patch % Lines
...c/components/GroupSystems/GroupImmutableSystems.js 2.63% 111 Missing ⚠️
...components/InventoryGroupDetail/GroupTabDetails.js 0.00% 14 Missing ⚠️
src/api/api.js 50.00% 3 Missing ⚠️
src/api/edge/imagesInfo.js 25.00% 3 Missing ⚠️
src/components/GroupSystems/index.js 25.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2102      +/-   ##
==========================================
- Coverage   59.22%   57.96%   -1.27%     
==========================================
  Files         188      189       +1     
  Lines        5938     6078     +140     
  Branches     1669     1700      +31     
==========================================
+ Hits         3517     3523       +6     
- Misses       2421     2555     +134     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gkarat gkarat added the enhancement New feature or request label Nov 22, 2023
@de1987
Copy link
Contributor

de1987 commented Nov 22, 2023

/retest

@acosferreira acosferreira marked this pull request as ready for review November 22, 2023 15:24
@acosferreira acosferreira requested a review from a team as a code owner November 22, 2023 15:24
@gkarat gkarat changed the title Theedge 3703 add system fix(THEEDGE-3703): On immutable tab we can't see the add system option Nov 22, 2023
@gkarat gkarat added the bug Something isn't working label Nov 22, 2023
const { uninitialized, loading, data } = useSelector(
(state) => state.groupDetail
);

return uninitialized || loading ? (
<EmptyState>
<EmptyStateBody>
<Spinner />
</EmptyStateBody>
</EmptyState>
) : (data?.results?.[0]?.host_count || 0) > 0 ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we double-check that data?.results?.[0]?.host_count also includes the immutable systems? If not, then we have to update the condition here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the results not includes the immutable.
We need explicit pass the filter to get the total of immutable.

Copy link
Contributor

Choose a reason for hiding this comment

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

But imagine the scenario: there are 0 conventional systems available, and there is at least one immutable system for this group. In that case, the app will show NoSystemsEmptyState because data?.results?.[0]?.host_count evaluates to 0 even though there are some immutable devices.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that is the expected behavior.
If there the total of conventional is 0 and immutable > 0 we should open the screen with focus in the immutable tab and the conventional will be empty.
empty

src/components/GroupSystems/index.js Outdated Show resolved Hide resolved
const { uninitialized, loading, data } = useSelector(
(state) => state.groupDetail
);

return uninitialized || loading ? (
<EmptyState>
<EmptyStateBody>
<Spinner />
</EmptyStateBody>
</EmptyState>
) : (data?.results?.[0]?.host_count || 0) > 0 ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

But imagine the scenario: there are 0 conventional systems available, and there is at least one immutable system for this group. In that case, the app will show NoSystemsEmptyState because data?.results?.[0]?.host_count evaluates to 0 even though there are some immutable devices.

src/components/InventoryGroupDetail/GroupTabDetails.js Outdated Show resolved Hide resolved
src/components/InventoryGroupDetail/GroupTabDetails.js Outdated Show resolved Hide resolved
src/components/InventoryGroupDetail/GroupTabDetails.js Outdated Show resolved Hide resolved
src/components/InventoryGroupDetail/GroupTabDetails.js Outdated Show resolved Hide resolved
src/components/GroupSystems/GroupImmutableSystems.js Outdated Show resolved Hide resolved
src/components/GroupSystems/GroupImmutableSystems.js Outdated Show resolved Hide resolved
@acosferreira
Copy link
Contributor Author

/retest

Copy link
Contributor

@gkarat gkarat left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you @acosferreira

@gkarat gkarat merged commit 2b6439c into RedHatInsights:master Nov 27, 2023
1 of 2 checks passed
@gkarat
Copy link
Contributor

gkarat commented Nov 27, 2023

🎉 This PR is included in version 1.60.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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 released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants