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

🐛 Disabling useFetchCache query if RWX_SUPPORTED=false #1963

Merged
merged 2 commits into from
Oct 30, 2024

Conversation

mguetta1
Copy link
Collaborator

@mguetta1 mguetta1 commented Jun 17, 2024

Copy link

codecov bot commented Jun 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 41.98%. Comparing base (b654645) to head (ee7c7a2).
Report is 241 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1963      +/-   ##
==========================================
+ Coverage   39.20%   41.98%   +2.78%     
==========================================
  Files         146      175      +29     
  Lines        4857     5630     +773     
  Branches     1164     1409     +245     
==========================================
+ Hits         1904     2364     +460     
- Misses       2939     3145     +206     
- Partials       14      121     +107     
Flag Coverage Δ
client 41.98% <100.00%> (+2.78%) ⬆️
server ?

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link
Member

@sjd78 sjd78 left a comment

Choose a reason for hiding this comment

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

@mguetta1 - Can you provide some context for this change? Is there an issue or ticket for this? At face value it looks ok, but I'm not sure how the isRWXSupported flag is meant to interact with the loading spinner.

@mguetta1
Copy link
Collaborator Author

@mguetta1 - Can you provide some context for this change? Is there an issue or ticket for this? At face value it looks ok, but I'm not sure how the isRWXSupported flag is meant to interact with the loading spinner.

Hi @sjd78, @dymurray
I just opened a Jira ticket for this issue: https://issues.redhat.com/browse/MTA-3699

Copy link
Collaborator

@rszwajko rszwajko left a comment

Choose a reason for hiding this comment

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

The loading state seems the consequence of the query being active. IMHO if the entire feature is disabled we should disable the query as well.
React query uses enable flag for this purpose. We use in several places i.e. here.

@mguetta1 mguetta1 requested a review from rszwajko October 7, 2024 07:15
@mguetta1
Copy link
Collaborator Author

mguetta1 commented Oct 7, 2024

The loading state seems the consequence of the query being active. IMHO if the entire feature is disabled we should disable the query as well. React query uses enable flag for this purpose. We use in several places i.e. here.

Done in ee7c7a2
@rszwajko better to pass isRWXSupported as an argument? I saw that useFetchCache is used only in client/src/app/pages/repositories/Mvn.tsx

Copy link
Collaborator

@rszwajko rszwajko left a comment

Choose a reason for hiding this comment

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

LGTM! If there will be another use case for useFetchCache then we can always refactor.

@rszwajko
Copy link
Collaborator

rszwajko commented Oct 7, 2024

@mguetta1 please edit the title to better reflect the fix.

@mguetta1 mguetta1 changed the title 🐛 Enable mvn spinner if isRWXSupported 🐛 Disabling useFetchCache query if RWX_SUPPORTED=false Oct 7, 2024
@sjd78 sjd78 merged commit 0cfe60b into konveyor:main Oct 30, 2024
14 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
Development

Successfully merging this pull request may close these issues.

3 participants