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.

Sign up for GitHub

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

Jump to bottom

feat(zones): fetch zones using react-query MAASENG-3404 #5485

Merged
merged 1 commit into from
Jul 3, 2024
Merged

feat(zones): fetch zones using react-query MAASENG-3404 #5485

merged 1 commit into from
Jul 3, 2024

Conversation

petermakowski
Copy link
Contributor

@petermakowski petermakowski commented Jun 25, 2024 •
edited
Loading

Done

This PR introduces React Query for managing zone-related data fetching and caching.

  • Add WebSocketEndpoints detailing allowed WebSocket endpoint models and methods
  • Move from Redux state management to React Query for handling zones-related data
  • Removed unused imports and code related to fetching zones
  • Refactored code to use new useZoneById hook for fetching zones by ID
  • Update testing utilities to support React Query and WebSocket testing
  • Add new helper functions in testing/utils for setting up initial state and query data
  • Modified renderWithBrowserRouter to return store and queryClient for more concise tests

QA steps

  • Go to AZs page and verify zones are fetched correctly using react-query
  • Add a new zone
  • Verify it appears on the list
  • Check that all zone-related components display data properly
  • Test websocket functionality with zone updates
  • Ensure msw is working as expected in development environment

Fixes

Fixes: MAASENG-3404

@webteam-app
Copy link

Demo

Jenkins

demos.haus

@petermakowski petermakowski force-pushed the zones-list-react-query-MAASENG-3404 branch 4 times, most recently from 27fc75b to 6a27b1a Compare June 27, 2024 13:50
@petermakowski petermakowski changed the title refactor(zones): react query MAASENG-3404 feat(zones): fetch zones using react-query MAASENG-3404 Jun 27, 2024
@petermakowski petermakowski force-pushed the zones-list-react-query-MAASENG-3404 branch 24 times, most recently from 119a8bb to 08def15 Compare July 2, 2024 10:12
@petermakowski petermakowski force-pushed the zones-list-react-query-MAASENG-3404 branch 3 times, most recently from 25b9078 to c5b20bc Compare July 2, 2024 13:10
@petermakowski petermakowski requested review from ndv99 and tmerten July 2, 2024 13:11
@petermakowski petermakowski marked this pull request as ready for review July 2, 2024 13:12
@ndv99
Copy link
Contributor

ndv99 commented Jul 2, 2024

Looks like lint is failing on this at the moment

@petermakowski
Copy link
Contributor Author

Looks like lint is failing on this at the moment

Please have a look regardless, this PR is pretty big. :)

@petermakowski petermakowski force-pushed the zones-list-react-query-MAASENG-3404 branch 3 times, most recently from 7b10cef to 5ad8dac Compare July 2, 2024 15:03
ndv99
ndv99 reviewed Jul 3, 2024
View reviewed changes
src/app/zones/views/ZoneDetails/ZoneDetailsHeader/ZoneDetailsHeader.test.tsx
});

const { findByText } = within(
await screen.findByTestId("section-header-title")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a specific reason that you're using findBy instead of getBy in these tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

findBy is an equivalent of calling waitFor(() => expect(() => screen.getBy

https://testing-library.com/docs/dom-testing-library/api-async#findby-queries

src/app/store/machine/slice.ts
payload: {
params: { system_ids: ids },
},
}) as const,
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the rationale for adding all of these as const in the slices?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this, the type for websocket actions ended up being string instead of a literal. Adding as const ensures that the action type/method is inferred as a specific string literal rather than the more general string type.

@ndv99
Copy link
Contributor

ndv99 commented Jul 3, 2024

QA is looking good. New zones / modified zones have a bit of delay in waiting for the changes to propagate, but that might just be my slow connection

@petermakowski petermakowski force-pushed the zones-list-react-query-MAASENG-3404 branch 3 times, most recently from 1adbed8 to 6657f2c Compare July 3, 2024 09:42
ndv99
ndv99 approved these changes Jul 3, 2024
View reviewed changes
Copy link
Contributor

@ndv99 ndv99 left a comment

Choose a reason for hiding this comment

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

LGTM, should be good to merge as soon as CI passes 👍

@petermakowski petermakowski force-pushed the zones-list-react-query-MAASENG-3404 branch 3 times, most recently from 06c4793 to 8b4bfc4 Compare July 3, 2024 11:47
@petermakowski petermakowski force-pushed the zones-list-react-query-MAASENG-3404 branch from 8b4bfc4 to bf39ecf Compare July 3, 2024 13:14
@petermakowski petermakowski merged commit 9215ed6 into canonical:main Jul 3, 2024
8 checks passed
@petermakowski petermakowski deleted the zones-list-react-query-MAASENG-3404 branch July 3, 2024 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ndv99 ndv99 ndv99 approved these changes

@tmerten tmerten Awaiting requested review from tmerten

Assignees
No one assigned
Labels
Review: Code +1 Review: QA +1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@petermakowski @webteam-app @ndv99