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

changed all dispatch requests with useQuery and request in asset module #6370

Closed
wants to merge 5 commits into from
Closed

changed all dispatch requests with useQuery and request in asset module #6370

wants to merge 5 commits into from

Conversation

kshitijv256
Copy link
Contributor

@kshitijv256 kshitijv256 commented Sep 29, 2023 •
edited by ghost
Loading

WHAT

🤖 Generated by Copilot at 4f06879

The pull request refactors the code for various components related to assets management, using the request utility function and the useQuery hook for making API calls instead of the dispatch action and the action creators. This simplifies the code, reduces the number of imports, and fixes some issues with the API paths and parameters. The pull request also adds response type definitions for the asset and facility API routes in the src/Redux/api.tsx file, using the Res generic function and the imported interfaces.

Proposed Changes

@coronasafe/care-fe-code-reviewers @coronasafe/code-reviewers

HOW

🤖 Generated by Copilot at 4f06879

@kshitijv256 kshitijv256 requested a review from a team September 29, 2023 11:02
@kshitijv256 kshitijv256 requested a review from a team as a code owner September 29, 2023 11:02
@vercel
Copy link

vercel bot commented Sep 29, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
care-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 29, 2023 1:14pm

@netlify
Copy link

netlify bot commented Sep 29, 2023 •
edited
Loading

✅ Deploy Preview for care-egov-staging ready!

Name Link
🔨 Latest commit aae1541
🔍 Latest deploy log https://app.netlify.com/sites/care-egov-staging/deploys/6516cd8c2ed9f200089d1d8e
😎 Deploy Preview https://deploy-preview-6370--care-egov-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

rithviknishad
rithviknishad requested changes Sep 29, 2023
View reviewed changes
Copy link
Member

@rithviknishad rithviknishad left a comment

Choose a reason for hiding this comment

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

You'll also have to remove the no longer used actions as mentioned in the EPIC issue.

src/Redux/api.tsx
},
createAssetBed: {
path: "/api/v1/assetbed/",
method: "POST",
TRes: Res<AssetData>(),
Copy link
Member

Choose a reason for hiding this comment

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

You'll also have to add TBody for all requests that will have a body.

src/Redux/api.tsx
@@ -238,6 +251,7 @@ const routes = {
partialUpdateAssetBed: {
path: "/api/v1/assetbed/{external_id}/",
method: "PATCH",
TRes: Res<AssetBedModel>(),
Copy link
Member

Choose a reason for hiding this comment

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

TBody here too...

src/Redux/api.tsx
@@ -818,13 +834,15 @@ const routes = {
partialUpdateAsset: {
path: "/api/v1/asset/{external_id}/",
method: "PATCH",
TRes: Res<AssetData>(),
Copy link
Member

Choose a reason for hiding this comment

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

TBody here too..

src/Redux/api.tsx
@@ -844,6 +863,7 @@ const routes = {
updateAssetService: {
path: "/api/v1/asset/{asset_external_id}/service_records/{external_id}",
method: "PUT",
TRes: Res<AssetService>(),
Copy link
Member

Choose a reason for hiding this comment

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

TBody here too...

src/Components/Assets/AssetFilter.tsx
}
}
} else {
setLocation(initialLocation);
}
},
[filter.location]
[facilityId, locationId]
);

useAbortableEffect((status: statusType) => {
Copy link
Member

Choose a reason for hiding this comment

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

useAbortableEffect is no longer needed

src/Components/Assets/AssetsList.tsx
Comment on lines +89 to +91
if (data) {
setSelectedFacility(data);
}
Copy link
Member

Choose a reason for hiding this comment

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

you won't need this state when using useQuery

src/Components/Assets/AssetsList.tsx
);
useEffect(() => {
async function fetchFacilityName() {
if (!qParams.facility) return setFacilityName("");
const res = await dispatch(getAnyFacility(qParams.facility, "facility"));
setFacilityName(res?.data?.name);
const { data } = await request(routes.getAnyFacility, {
Copy link
Member

Choose a reason for hiding this comment

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

replace with useQuery

src/Components/Assets/AssetsList.tsx

const fetchFacility = useCallback(
async (status: statusType) => {
if (!qParams.facility) return setFacility(undefined);
setIsLoading(true);
const res = await dispatch(getAnyFacility(qParams.facility));
const res = await request(routes.getAnyFacility, {
Copy link
Member

Choose a reason for hiding this comment

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

replace with useQuery

src/Components/Assets/AssetsList.tsx
const res = await dispatch(
getFacilityAssetLocation(qParams.facility, qParams.location)
);
const { data } = await request(routes.getFacilityAssetLocation, {
Copy link
Member

Choose a reason for hiding this comment

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

replace with useQuery

src/Components/Assets/AssetsList.tsx
setIsLoading(false);
}
},
[dispatch, qParams.facility, qParams.location]
[qParams.facility, qParams.location]
);

useAbortableEffect(
Copy link
Member

Choose a reason for hiding this comment

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

useAbortableEffect is not needed when using request/useQuery

rithviknishad
rithviknishad requested changes Sep 29, 2023
View reviewed changes
src/Redux/api.tsx
@@ -171,10 +177,14 @@ const routes = {

getPermittedFacility: {
path: "/api/v1/facility/{id}/",
method: "GET",
TRes: Res<FacilityModel>(),
Copy link
Member

Choose a reason for hiding this comment

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

Rebase with latest develop branch, the Res has been renamed to Type to make it generic for use with TBody too...

@rithviknishad
Copy link
Member

@kshitijv256 when making changes, feel free to split the entire refactor into smaller PRs if that would help.

It would be easier to make the changes, QA testing, and review.

@kshitijv256 kshitijv256 mentioned this pull request Sep 29, 2023
Merged
@kshitijv256
Copy link
Contributor Author

@rithviknishad made another PR #6374 with requested changes

@kshitijv256 kshitijv256 deleted the assets_update branch December 4, 2023 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rithviknishad rithviknishad rithviknishad requested changes

@vigneshhari vigneshhari Awaiting requested review from vigneshhari

Assignees
No one assigned
Labels
changes required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

🛠️ Replace useDispatch w. useQuery/request: Assets (src/Components/Assets/**)
2 participants
@kshitijv256 @rithviknishad