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

replace useDispatch with useQuery/request in facility module (Part 2, E-Z) (src/Components/Facility/[E-Z]*.tsx #6448

Closed
wants to merge 13 commits into from

Conversation

sriharsh05
Copy link
Contributor

@sriharsh05 sriharsh05 commented Oct 14, 2023

WHAT

🤖 Generated by Copilot at 39dcf7c

No summary available (Limit exceeded: required to process 63082 tokens, but only 50000 are allowed per call)

The pull request partially refactors the codebase for managing facility module 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 improves the code quality, performance, and readability by removing unnecessary imports, variables, and dependencies.

Proposed Changes

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

Merge Checklist

  • Add specs that demonstrate bug / test a new feature.
  • Update product documentation.
  • Ensure that UI text is kept in I18n files.
  • Prep screenshot or demo video for changelog entry, and attach it to issue.
  • Request for Peer Reviews
  • Completion of QA

HOW

🤖 Generated by Copilot at 39dcf7c

No walkthrough available (Limit exceeded: required to process 63082 tokens, but only 50000 are allowed per call)

@sriharsh05 sriharsh05 requested a review from a team October 14, 2023 12:12
@sriharsh05 sriharsh05 requested a review from a team as a code owner October 14, 2023 12:12
@vercel
Copy link

vercel bot commented Oct 14, 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 Nov 7, 2023 6:55am

@netlify
Copy link

netlify bot commented Oct 14, 2023

Deploy Preview for care-egov-staging ready!

Name Link
🔨 Latest commit 74024f6
🔍 Latest deploy log https://app.netlify.com/sites/care-egov-staging/deploys/6549df2991fc4f0008cc3e41
😎 Deploy Preview https://deploy-preview-6448--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.

@netlify
Copy link

netlify bot commented Oct 14, 2023

Deploy Preview for care-net failed.

Name Link
🔨 Latest commit 39dcf7c
🔍 Latest deploy log https://app.netlify.com/sites/care-net/deploys/652a859a018f9e0008234815

@github-actions github-actions bot added the Deploy-Failed Deplyment is not showing preview label Oct 14, 2023
@github-actions
Copy link

👋 Hi, @sriharsh05,
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.

@rithviknishad rithviknishad removed the Deploy-Failed Deplyment is not showing preview label Oct 17, 2023
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.

  1. Paginated Responses needs to be wrapped with PaginatedResponse<ModelName> instead of repeatedly redefining the paginated response's interface.

  2. Do not create new interfaces when such interfaces are already defined elsewhere in the codebase. Re-use those existing interfaces instead.

  3. Avoid using any type.

@sriharsh05 sriharsh05 changed the title replace useDispatch with useQuery/request in facility module replace useDispatch with useQuery/request in facility module (Part 2, E-Z) (src/Components/Facility/[E-Z]*.tsx Oct 22, 2023
@sriharsh05
Copy link
Contributor Author

hey @rithviknishad
Will you please review the changes?

@nihal467
Copy link
Member

@sriharsh05 clear the merge conflict

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.

@sriharsh05 could you clear the merge conflicts?

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

👋 Hi, @sriharsh05,
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.

@sriharsh05
Copy link
Contributor Author

@rithviknishad
Will you please review the changes?

@github-actions github-actions bot removed the merge conflict pull requests with merge conflict label Nov 3, 2023
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.

This PR is hard to review and test! Too many things can break and would be hard to test.

@sriharsh05 Feel free to make the requested changes in multiple new smaller PR's each targeting 1-3 files max.

package-lock.json Outdated Show resolved Hide resolved
src/Components/Assets/AssetFilter.tsx Show resolved Hide resolved
src/Components/Assets/AssetFilter.tsx Show resolved Hide resolved
src/Components/Assets/AssetFilter.tsx Show resolved Hide resolved
src/Components/Assets/AssetFilter.tsx Show resolved Hide resolved
Comment on lines +425 to +429
export type IInventoryLogResponse = Omit<
IInventorySummaryResponse,
"results"
> & {
results: {
Copy link
Member

Choose a reason for hiding this comment

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

You are omitting results and adding results of different type. Define a new type instead since this approach will simply make the code hard to read and hard to maintain

Comment on lines +457 to +472
interface FacilityObjectModel {
id: string;
name: string;
local_body: number;
district: number;
state: number;
ward_object: WardModel;
local_body_object: { id: number } & LocalBodyModel;
district_object: DistrictModel;
state_object: StateModel;
facility_type: string;
read_cover_image_url: string;
features: number[];
patient_count: string;
bed_count: string;
}
Copy link
Member

Choose a reason for hiding this comment

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

Why redefine FacilityModel in so many different ways? Just use the one already defined FacilityModel right?

Comment on lines +515 to +526
location_object: {
id: string;
facility: {
id: string;
name: string;
};
created_date: string;
modified_date: string;
name: string;
description: string;
location_type: number;
};
Copy link
Member

Choose a reason for hiding this comment

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

Map this to use LocationModel instead

Suggested change
location_object: {
id: string;
facility: {
id: string;
name: string;
};
created_date: string;
modified_date: string;
name: string;
description: string;
location_type: number;
};
location_object: LocationModel;

Comment on lines +545 to +556
location_object: {
id: string;
facility: {
id: string;
name: string;
};
created_date: string;
modified_date: string;
name: string;
description: string;
location_type: number;
};
Copy link
Member

Choose a reason for hiding this comment

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

Use LocationModel instead

Suggested change
location_object: {
id: string;
facility: {
id: string;
name: string;
};
created_date: string;
modified_date: string;
name: string;
description: string;
location_type: number;
};
location_object: LocationModel;

Comment on lines +557 to +565
last_service: {
id: string;
edits: ServiceEdit[];
external_id: string;
created_date: string;
modified_date: string;
serviced_on: string;
note: string;
};
Copy link
Member

Choose a reason for hiding this comment

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

This should point to an already defined model instead. Avoid redefing types again and again.

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

github-actions bot commented Nov 6, 2023

👋 Hi, @sriharsh05,
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.

@rithviknishad
Copy link
Member

Closing as smaller PRs have been made for the same

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes required merge conflict pull requests with merge conflict
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🛠️ Replace useDispatch w. useQuery/request: Facility (Part 2, E-H) (src/Components/Facility/[E-H]*.tsx)
3 participants