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

convert fetch api call to rtk mutation #529

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

convert fetch api call to rtk mutation #529

wants to merge 3 commits into from

Conversation

mayurlalwani
Copy link

@mayurlalwani mayurlalwani commented May 5, 2023 •
edited

  • Created mutation for update card content.
  • Converted fetch api call of update card content to RTK in helper function.
  • Wrote test for RTK.

@mayurlalwani mayurlalwani self-assigned this May 5, 2023
@vercel
Copy link

vercel bot commented May 5, 2023

@mayurlalwani is attempting to deploy a commit to the RDS-Team Team on Vercel.

A member of the Team first needs to authorize it.

@mayurlalwani mayurlalwani linked an issue May 5, 2023 that may be closed by this pull request
write RTK Mutation for updateCardContent API call #512
Closed
bhtibrewal
bhtibrewal reviewed May 6, 2023
View reviewed changes
Copy link
Member

@bhtibrewal bhtibrewal left a comment

Choose a reason for hiding this comment

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

Please write tests for the functionality, and description for the task.

src/app/services/tasksApi.ts Outdated
@@ -18,7 +19,20 @@ export const tasksApi = api.injectEndpoints({
query: () => '/tasks/mine',
providesTags: ['Tasks'],
}),
updateCardContent: builder.mutation({
Copy link
Member

Choose a reason for hiding this comment

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

What do you think will this file would be the right place to write this API?

Copy link
Author

Choose a reason for hiding this comment

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

As this component was related to tasks, so I added this API in this file. Should I create a new file?

Copy link
Author

Choose a reason for hiding this comment

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

Also, the unit test for the funcitonality already exists. Should I write unit test for this helper function - "updateCardContent"?

Copy link
Member

Choose a reason for hiding this comment

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

the file is okay but the naming doesn't clearly indicate what this function does, read what it is doing here

url: `"${TASKS_URL}/${id}"`,
                    method: 'PATCH',

Copy link
Member

Choose a reason for hiding this comment

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

yes, we are writing tests for this RTK hook also "updateCardContent" and the unit tests will also change because of the change in the implementation of the API call.

@mayurlalwani mayurlalwani marked this pull request as draft May 9, 2023 12:32
@mayurlalwani mayurlalwani force-pushed the 512-write-rtk-mutation-for-updatecardcontent-api-call branch from 47d7e04 to e9a18d3 Compare May 10, 2023 13:50
Pratiyushkumar
Pratiyushkumar suggested changes May 10, 2023
View reviewed changes
Copy link
Contributor

@Pratiyushkumar Pratiyushkumar left a comment

Choose a reason for hiding this comment

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

Please address the change request mentioned by bhavika and also write the test cases for it

@bhtibrewal bhtibrewal added the RTK label May 11, 2023
@mayurlalwani mayurlalwani force-pushed the 512-write-rtk-mutation-for-updatecardcontent-api-call branch from e9a18d3 to 0942865 Compare May 12, 2023 10:01
@mayurlalwani mayurlalwani force-pushed the 512-write-rtk-mutation-for-updatecardcontent-api-call branch from 0942865 to b834d29 Compare May 12, 2023 11:36
@mayurlalwani mayurlalwani marked this pull request as ready for review May 12, 2023 11:38
Pratiyushkumar
Pratiyushkumar previously approved these changes May 13, 2023
View reviewed changes
Copy link
Contributor

@Pratiyushkumar Pratiyushkumar left a comment

Choose a reason for hiding this comment

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

Everything looks good to me

bhtibrewal
bhtibrewal reviewed May 13, 2023
View reviewed changes
__tests__/Unit/hooks/useUpdateTask.test.tsx Outdated
@@ -95,4 +105,58 @@ describe('useUpdateTask hook', () => {
expect(setTasks).toHaveBeenCalled();
expect(setTasks).toHaveBeenLastCalledWith(groupedTasks);
});

test('Should update task details by id', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

please make a separate file for this , it should not go in this file

bhtibrewal
bhtibrewal reviewed May 13, 2023
View reviewed changes
__tests__/Unit/hooks/useUpdateTask.test.tsx Outdated
Comment on lines 122 to 145
id: '1',
cardDetails: {
id: '123',
lossRate: {
dinero: 10,
neelam: 5,
},
links: [''],
completionAward: {
dinero: 110,
neelam: 10,
},
dependsOn: [],
assignee: 'john',
startedOn: '1618790400',
isNoteworthy: true,
title: 'Test',
purpose: '',
percentCompleted: 0,
endsOn: '1618790400',
status: 'progress',
featureUrl: 'progress',
type: 'feature',
createdBy: 'sam',
Copy link
Member

Choose a reason for hiding this comment

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

Taking out this data into a variable will make the test easily readable

bhtibrewal
bhtibrewal reviewed May 13, 2023
View reviewed changes
src/components/tasks/TaskList/TaskList.tsx Outdated
function onSeeMoreTasksHandler() {
setTasksLimit((prevLimit) => prevLimit + ADD_MORE_TASKS_LIMIT);
}
async function onContentChangeHandler(id: string, cardDetails: any) {
if (!isEditable || !updateCardContent) return;
updateCardContent(id, cardDetails);
if (!isEditable || !updateTaskDetailsById) return;
Copy link
Member

Choose a reason for hiding this comment

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

please check is !updateTaskDetailsById required here

Copy link
Author

Choose a reason for hiding this comment

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

It is not required, I will remove it.

@mayurlalwani mayurlalwani force-pushed the 512-write-rtk-mutation-for-updatecardcontent-api-call branch from f3cc656 to 8888916 Compare May 13, 2023 16:29
Pratiyushkumar
Pratiyushkumar reviewed May 16, 2023
View reviewed changes
Copy link
Contributor

@Pratiyushkumar Pratiyushkumar left a comment

Choose a reason for hiding this comment

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

Looks good to me, ask Bhavika to approve, I will approve after she approves

@mayurlalwani mayurlalwani force-pushed the 512-write-rtk-mutation-for-updatecardcontent-api-call branch from 8888916 to e581013 Compare May 23, 2023 16:42
@mayurlalwani mayurlalwani force-pushed the 512-write-rtk-mutation-for-updatecardcontent-api-call branch from e581013 to 1462973 Compare May 28, 2023 12:08
@bhtibrewal bhtibrewal mentioned this pull request Jun 30, 2023
Open
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pratiyushkumar Pratiyushkumar Pratiyushkumar left review comments

@bhtibrewal bhtibrewal Awaiting requested review from bhtibrewal

At least 2 approving reviews are required to merge this pull request.

Assignees

@mayurlalwani mayurlalwani

Labels
RTK
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

write RTK Mutation for updateCardContent API call
3 participants
@mayurlalwani @bhtibrewal @Pratiyushkumar