Feat :Implement & Apply API client method for rename group feature #242

kanta1207 · 2024-02-19T03:04:45Z

Overview

Implement rename api client, and apply it to RenameGroupForm.

Changes

Implement api client for rename group feature.
Refactor create group api client, using result-ts-type.
Add action and schema files for create & rename group.
Add test for group api client methods and action

Review points

If the code is easy to read, well commented
If create and rename group api client are working properly

Assignee Checklist:

The naming convention of the PR title is correct (See the comment at the top of this template)
The base branch is correct (See: Types of Branches)
The branch name follows the Branch Naming Conventions
The correct assignees and reviewers have been designated for this PR
The coding style follows the Coding Style Guide
All the related issues are associated with this PR
All criteria in the associated issues are met (please tick the checkboxes)
My changes do not generate new warnings or errors (especially in the console)

Reviewer Checklist:

Double-check the "Assignee Checklist"
The code follows the generic best practices and our Coding Style Guide
The code is well-commented and easy to understand
The UI changes accurately reflect the provided design

kanta1207 · 2024-02-19T03:53:30Z

src/lib/api/group/client/groupApiClient.client.ts

 */
-export const createGroup = async (params: ICreateGroupParams): Promise<string> => {
+export const postCreateGroup = async (
+  payload: IPostCreateGroupPayload,


Just changed the variable name from params to payload because it's defined as payload in web api doc

nick-y-ito

@kanta1207
Thank you for your work! Overall, your implementation has no problem!
As for code styling and conventions, I added some comments. Please take a look when you have a moment.

By the way, your code reminded me that I missed TSDocs for actions and API clients of the foods feature. I will fix them in my next PR

nick-y-ito · 2024-02-19T15:20:26Z

src/features/groups/components/GroupCardList/GroupCard/GroupCardContent/RenameGroupForm.tsx

  /**
   * Process the form submission.
   * @param values - The form values
   */
-  const processSubmit: SubmitHandler<Inputs> = async (values) => {
+  const processSubmit: SubmitHandler<RenameGroupInputs> = async (values) => {


Can you please avoid using nested if statements?

nick-y-ito · 2024-02-19T15:21:28Z

src/features/groups/components/CreateGroupButton/CreateGroupDrawerContent.tsx


 import { zodResolver } from '@hookform/resolvers/zod';
 import { FC, useEffect } from 'react';
 import { SubmitHandler, useForm } from 'react-hook-form';
 import { z } from 'zod';

+import { createGroup } from '../../lib/actions';
+import { createGroupFormSchema, CreateGroupInputs } from '../../lib/schemas';


Could you please avoid using relative paths except it's in the same directory ./?

nick-y-ito · 2024-02-19T17:51:18Z

src/features/groups/lib/actions.ts

+} from './schemas';
+
+/**
+ * Method to create a group.


The term "method" is only used to describe functions that are defined in an object or a class. There are several more lines that use the word "method" incorrectly.

nick-y-ito · 2024-02-19T18:03:45Z

src/features/groups/lib/actions.ts

+
+/**
+ * Method to create a group.
+ * @param inputs - The payload for the method.


In terms of API, the word "payload" seems the actual data transported between the backend and the frontend.

Simply put, it is the body of your HTTP request and response message.

Since the param inputs here is just plain user's inputs, using the word "payload" is technically incorrect.

See: What is Payload?

*There are a few more lines that use the word "payload" in the wrong way.

Yeah you're right, inputs in here is just user inputs... I renamed them!

nick-y-ito · 2024-02-19T18:44:33Z

src/lib/api/group/client/groupApiClient.client.ts

I like how you organized this file. I will follow your conversions in my next PR.

nick-y-ito · 2024-02-19T19:08:52Z

src/lib/api/group/client/groupApiClient.client.test.ts

jest.mock('@/lib/api/common/client/commonUtils.client', () => ({

to

jest.mock('@/lib/api/common/client', () => ({

It would be more helpful if you could explicitly state the targets of this test.

/** * @template * @return */

These are non-standard @tags since they are not defined in the official TSDoc documents. The correct usage is @returns (plural) instead of @return. If you find more in other files, could you please fix them together?

@kanta1207 Thank you for fixing it. However, the @template is not a standard tag, either. Could you find all and fix them, please?

nick-y-ito · 2024-02-19T19:10:22Z

src/lib/api/group/client/groupApiClient.client.test.ts

+  IPostCreateGroupApiResponse,
+  IPostCreateGroupPayload,
+  putRenameGroup,
+} from './groupApiClient.client';


Combine the imports:

import { postCreateGroup } from '@/lib/api/group/client';

import { IPostCreateGroupApiResponse, IPostCreateGroupPayload, putRenameGroup, } from './groupApiClient.client';

You're right, and I also forgot to index the exports...
I fixed them!

nick-y-ito · 2024-02-19T19:32:11Z

src/features/groups/lib/actions.test.ts

+      const result = await createGroup(mockInvalidInputs);
+
+      // Assert
+      expect(result).toEqual(Err('Validation failed'));


It would be better to include this assertion. Because calling the postCreateGroup before the error is thrown might result in unintentional creation of data.

expect(postCreateGroup).not.toHaveBeenCalled();

nick-y-ito · 2024-02-19T19:33:40Z

src/features/groups/lib/actions.test.ts

+      const result = await renameGroup(mockGroupId, mockInvalidInputs);
+
+      // Assert
+      expect(result).toEqual(Err('Validation failed'));


Same for here.

nick-y-ito · 2024-02-19T19:54:39Z

src/lib/api/group/client/groupApiClient.client.test.ts

-    };
+  describe('postCreateGroup', () => {
+    // Arrange mock data
+    const mockPayload: IPostCreateGroupPayload = { groupName: 'Shared-house' };
    it('successfully creates a group', async () => {


Could you add a line break before each it() and describe to improve readability, please?

@kanta1207 Thank you for fixing it, but I found one in the src/features/groups/lib/actions.test.ts, too.

kanta1207 · 2024-02-20T00:54:12Z

@nick-y-ito
Thank you for your review!
I addressed the issues you mentioned, so please take a relook at them when you have a moment 🙏

nick-y-ito · 2024-02-20T14:50:57Z

@kanta1207
Thank you for addressing my previous comments. However, I noticed a few small issues that were not corrected. Could you please take a second look?

Feat :Implement & Apply API client method for rename group feature #242 (comment)
Feat :Implement & Apply API client method for rename group feature #242 (comment)

kanta1207 · 2024-02-20T15:33:02Z

@kanta1207 Thank you for addressing my previous comments. However, I noticed a few small issues that were not corrected. Could you please take a second look?

Feat :Implement & Apply API client method for rename group feature #242 (comment)

Feat :Implement & Apply API client method for rename group feature #242 (comment)

@nick-y-ito
Sorry for missing some points 🙏
I addressed them, please kindly take a look!

nick-y-ito

@kanta1207
Everything looks good to me now :)
Thank you very much for your excellent work!!

kanta1207 added 4 commits February 18, 2024 18:03

feat: add schema and action

400852d

test: test for putRenameGroup method

91433e9

feat: apply api function to rename form

6a25940

test: test actions

d86e94b

kanta1207 linked an issue Feb 19, 2024 that may be closed by this pull request

Create API client method for "Rename Group" feature #237

Closed

2 tasks

kanta1207 self-assigned this Feb 19, 2024

kanta1207 commented Feb 19, 2024

View reviewed changes

kanta1207 requested review from kotaaaa, nick-y-ito and ShoeheyOt February 19, 2024 04:01

kanta1207 marked this pull request as ready for review February 19, 2024 04:01

nick-y-ito requested changes Feb 19, 2024

View reviewed changes

nick-y-ito reviewed Feb 19, 2024

View reviewed changes

fix: fix comments and import paths

73b91ff

kanta1207 added 2 commits February 20, 2024 07:23

fix: remove @template tag

de5039a

fix: add a line before it in test file

4155212

nick-y-ito approved these changes Feb 20, 2024

View reviewed changes

kanta1207 merged commit 3707e99 into develop Feb 20, 2024
3 checks passed

kanta1207 deleted the feature/237-api-client-method-for-rename-group-feature branch February 20, 2024 15:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat :Implement & Apply API client method for rename group feature #242

Feat :Implement & Apply API client method for rename group feature #242

kanta1207 commented Feb 19, 2024 •

edited by nick-y-ito

Loading

kanta1207 Feb 19, 2024

nick-y-ito left a comment •

edited

Loading

nick-y-ito Feb 19, 2024

nick-y-ito Feb 19, 2024

nick-y-ito Feb 19, 2024

nick-y-ito Feb 19, 2024

kanta1207 Feb 20, 2024

nick-y-ito Feb 19, 2024

nick-y-ito Feb 19, 2024

nick-y-ito Feb 19, 2024

nick-y-ito Feb 19, 2024

nick-y-ito Feb 20, 2024

nick-y-ito Feb 19, 2024

kanta1207 Feb 20, 2024

nick-y-ito Feb 19, 2024

nick-y-ito Feb 19, 2024

nick-y-ito Feb 19, 2024

nick-y-ito Feb 20, 2024 •

edited

Loading

kanta1207 commented Feb 20, 2024

nick-y-ito commented Feb 20, 2024 •

edited

Loading

kanta1207 commented Feb 20, 2024

nick-y-ito left a comment

Feat :Implement & Apply API client method for rename group feature #242

Feat :Implement & Apply API client method for rename group feature #242

Conversation

kanta1207 commented Feb 19, 2024 • edited by nick-y-ito Loading

Overview

Changes

Review points

Assignee Checklist:

Reviewer Checklist:

Choose a reason for hiding this comment

nick-y-ito left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nick-y-ito Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

kanta1207 commented Feb 20, 2024

nick-y-ito commented Feb 20, 2024 • edited Loading

kanta1207 commented Feb 20, 2024

nick-y-ito left a comment

Choose a reason for hiding this comment

kanta1207 commented Feb 19, 2024 •

edited by nick-y-ito

Loading

nick-y-ito left a comment •

edited

Loading

nick-y-ito Feb 20, 2024 •

edited

Loading

nick-y-ito commented Feb 20, 2024 •

edited

Loading