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

Replaced dispatch with useQuery and request in Asset module #6374

Merged
merged 27 commits into from
Oct 18, 2023

Conversation

kshitijv256
Copy link
Contributor

@kshitijv256 kshitijv256 commented Sep 29, 2023

WHAT

🤖 Generated by Copilot at 08d6244

The pull request refactors the codebase for managing assets and asset services in the frontend. It replaces the redux actions and states with the custom request function and the useQuery hook for data fetching and updating. It also adds type annotations and validations for the asset-related API routes using runtypes. It improves the code quality, performance, and readability by removing unnecessary imports, variables, and dependencies.

Proposed Changes

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

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

vercel bot commented Sep 29, 2023

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 Oct 17, 2023 3:29am

@netlify
Copy link

netlify bot commented Sep 29, 2023

Deploy Preview for care-egov-staging ready!

Name Link
🔨 Latest commit fc2e15e
🔍 Latest deploy log https://app.netlify.com/sites/care-egov-staging/deploys/652dff670a06250008658890
😎 Deploy Preview https://deploy-preview-6374--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.

@kshitijv256 kshitijv256 changed the title changed to request in HL7monitor.tsx Replaced dispatch wiith useQuery and request in Asset module Sep 29, 2023
src/Components/Assets/AssetTypes.tsx Outdated Show resolved Hide resolved
src/Components/Assets/AssetFilter.tsx Outdated Show resolved Hide resolved
src/Components/Assets/AssetFilter.tsx Outdated Show resolved Hide resolved
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.

Remove all additional useState's used to store the response data as this is already achieved by useQuery itself and you can access the data directly instead of again creating another useState

src/Components/Assets/AssetManage.tsx Outdated Show resolved Hide resolved
src/Components/Assets/configure/MonitorConfigure.tsx Outdated Show resolved Hide resolved
@kshitijv256 kshitijv256 changed the title Replaced dispatch wiith useQuery and request in Asset module Replaced dispatch with useQuery and request in Asset module Oct 1, 2023
@nihal467 nihal467 added needs testing and removed cypress failed pull request with cypress test failure labels Oct 10, 2023
@nihal467
Copy link
Member

nihal467 commented Oct 10, 2023

@kshitijv256 https://deploy-preview-6374--care-net.netlify.app/facility/9447524f-6d2b-4b24-87dc-e30f68c6642c/assets/1ce11cbf-2398-4f9b-8828-6d416250bf15

image

we are having multiple asset with name "test-camera" under the facility test-oct-firstweek.

When we create a new asset with the name test-camera, the transaction history details are getting manipulated

image

@kshitijv256
Copy link
Contributor Author

@nihal467 fixed the problem please check. transaction filter was being passed incorrectly in request and all transaction data was shown in all assets

@kshitijv256
Copy link
Contributor Author

all the changes requested has been done

@nihal467
Copy link
Member

LGTM

@github-actions github-actions bot added the merge conflict pull requests with merge conflict label Oct 17, 2023
@github-actions
Copy link

👋 Hi, @kshitijv256,
Conflicts have been detected against the base branch. Please rebase your branch against the base branch.


This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there.

@khavinshankar
Copy link
Member

@kshitijv256 resolve the merge conflicts

@rithviknishad rithviknishad removed the merge conflict pull requests with merge conflict label Oct 17, 2023
@khavinshankar khavinshankar merged commit 9b362c1 into ohcnetwork:develop Oct 18, 2023
28 checks passed
Ashesh3 pushed a commit that referenced this pull request Oct 20, 2023
* changed to request in HL7monitor.tsx

* replaced dispatch in assetTypes

* changed dispatch in filteer, import files

* changed all dispatch requests with useQuery and request in asset module

* added TBody to various endpoints

* fixed assetfilter page

* fixed asset import modal page

* made requested changes

* made requested changes

* reverted to assetBedModel

* fixed delete asset function

* fixed after delete redirection

* removed extra setstates

* updated failing cypress tests for asset module

* removed useEffects

* removed changes from cypress files

* fixed merge issues

* fixed transactions issue

* remove console logs

----------

Co-authored-by: Rithvik Nishad <[email protected]>
Ashesh3 pushed a commit that referenced this pull request Oct 23, 2023
* changed to request in HL7monitor.tsx

* replaced dispatch in assetTypes

* changed dispatch in filteer, import files

* changed all dispatch requests with useQuery and request in asset module

* added TBody to various endpoints

* fixed assetfilter page

* fixed asset import modal page

* made requested changes

* made requested changes

* reverted to assetBedModel

* fixed delete asset function

* fixed after delete redirection

* removed extra setstates

* updated failing cypress tests for asset module

* removed useEffects

* removed changes from cypress files

* fixed merge issues

* fixed transactions issue

* remove console logs

----------

Co-authored-by: Rithvik Nishad <[email protected]>
@@ -43,18 +42,14 @@ const AssetImportModal = ({ open, onClose, facility }: Props) => {
setSelectedFile(undefined);
onClose && onClose();
};

useEffect(() => {
setLocationsLoading(true);
Copy link
Member

Choose a reason for hiding this comment

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

Why was this removed? The loader for when the locations are still being fetched does not work anymore.. setLocationsLoading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🛠️ Replace useDispatch w. useQuery/request: Assets (src/Components/Assets/**)
5 participants