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

AssignTask to rtk #699

Closed
wants to merge 23 commits into from
Closed

Conversation

WYGIN
Copy link

@WYGIN WYGIN commented Jul 9, 2023

Issue: #671

Description:

migrated AssignTask api from Axios to RTK & wrote Tests to check AssignTask RTK mutation

Anything you would like to inform the reviewer about:

Dev Tested:

  • Yes

Test Coverage

  • Yes

Images/video of the change:

AssignTaskToRTK.mp4

Picsart_23-07-17_18-30-44-898

Follow-up Issues (if any)

not due to my changes but the task assigned is not getting invisible until the next page reload if i try to assign first task, but the last task in the list is getting invisible after i assigned it.

@vercel
Copy link

vercel bot commented Jul 9, 2023

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

A member of the Team first needs to authorize it.

@WYGIN WYGIN changed the title Assign task to rtk AssignTask to rtk Jul 9, 2023
@WYGIN WYGIN marked this pull request as ready for review July 11, 2023 11:33
@sahsisunny
Copy link
Member

Please add the test coverage screenshot of the changes

@WYGIN WYGIN closed this Jul 15, 2023
@WYGIN WYGIN force-pushed the assignTaskToRTK branch 2 times, most recently from 4123e2b to bc04ca3 Compare July 15, 2023 10:42
@WYGIN WYGIN reopened this Jul 15, 2023
@sahsisunny
Copy link
Member

Add the test coverage report of your changes only @WYGIN

.eslintrc.json Outdated
@@ -18,7 +18,7 @@
"plugins": ["react", "@typescript-eslint"],
"rules": {
"linebreak-style": ["error", "unix"],
"quotes": ["error", "single"],
"quotes": ["error", "single", { "avoidEscape": true }],
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we adding this rule here?
We should follow one

Copy link
Author

Choose a reason for hiding this comment

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

image
if i doesn't add it to eslint, eslint is trying to escape character & prettier is trying to avoid escaping characters, resulting in git push failure when i try to push changes

Copy link
Contributor

@prakashchoudhary07 prakashchoudhary07 Jul 18, 2023

Choose a reason for hiding this comment

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

In that case, we should fix the prettier and eslint configurations, rather than just adding some rule to make precommits pass

Copy link
Author

Choose a reason for hiding this comment

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

In that case, we should fix the prettier and eslint configurations, rather than just adding some rule to make precommits pass

i searched a lot to fix that issue but not found any solution for it, so i reverted my code to use defalut lint rules

Copy link
Contributor

@bhtibrewal bhtibrewal Jul 19, 2023

Choose a reason for hiding this comment

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

if you face an issue that is not linked to your PR, please create a separate issue ticket and PR for it

@WYGIN WYGIN closed this Jul 17, 2023
@WYGIN WYGIN force-pushed the assignTaskToRTK branch 2 times, most recently from 73ab104 to 4bd3552 Compare July 17, 2023 07:11
@WYGIN WYGIN reopened this Jul 17, 2023
Comment on lines +5 to +23
id: 'aTOG168A86JXY5wVosJx',
github_display_name: 'Mahima Khandelwal',
github_id: 'Maheima',
roles: {
member: true,
archived: false,
super_user: false,
},
last_name: 'Khandelwal',
username: 'mahima',
incompleteUserDetails: false,
profileStatus: 'BLOCKED',
picture: {
publicId: 'profile/aTOG168A86JXY5wVosJx/fj2c46kpmpy3gi8tl63s',
url: 'https://res.cloudinary.com/realdevsquad/image/upload/v1674639637/profile/aTOG168A86JXY5wVosJx/fj2c46kpmpy3gi8tl63s.jpg'
},
status: 'active',
first_name: 'Mahima',
profileURL: 'https://mahima-profile-service.onrender.com'
Copy link
Member

Choose a reason for hiding this comment

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

Please use dummy data for test

Copy link
Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

Where?

Copy link
Author

@WYGIN WYGIN Jul 18, 2023

Choose a reason for hiding this comment

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

Where?

That data is not created by me i refactored it tor DRY them up but now i replaced my tests with duplicate data in path __ tests __/Unit/hooks/tasksApi.test.tsx so no more tests depending on that data in my tests, the file is renamed from a js file to ts file https://github.com/Real-Dev-Squad/website-status/pull/699/files#diff-6e68b838da6a1b199fcb4ef878d985dd3e29de628b6bbbcdaca7cf99de329d3e

@WYGIN WYGIN requested a review from sahsisunny July 18, 2023 07:35
Comment on lines 5 to 6
export const urlPattern =
/(https:\/\/www\.|http:\/\/www\.|https:\/\/|http:\/\/)?[a-zA-Z]{2,}(\.[a-zA-Z]{2,})(\.[a-zA-Z]{2,})?\/[a-zA-Z0-9]{2,}|((https:\/\/www\.|http:\/\/www\.|https:\/\/|http:\/\/)?[a-zA-Z]{2,}(\.[a-zA-Z]{2,})(\.[a-zA-Z]{2,})?)|(https:\/\/www\.|http:\/\/www\.|https:\/\/|http:\/\/)?[a-zA-Z0-9]{2,}\.[a-zA-Z0-9]{2,}\.[a-zA-Z0-9]{2,}(\.[a-zA-Z0-9]{2,})?/g;
Copy link
Contributor

Choose a reason for hiding this comment

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

What pattern are we matching here?

Copy link
Author

@WYGIN WYGIN Jul 19, 2023

Choose a reason for hiding this comment

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

What pattern are we matching here?

i changed that pattern with a simple relative url pattern please check it

expect(unAssignedTaskNodes).toHaveLength(10);
expect(unAssignedTaskNodes[0]).toBeVisible();
expect(unAssignedTaskNodes[6]).toBeVisible();
getByText('Akash Shukla');
Copy link
Contributor

Choose a reason for hiding this comment

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

didn't understand why is this written at the end.

Copy link
Author

@WYGIN WYGIN Jul 19, 2023

Choose a reason for hiding this comment

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

didn't understand why is this written at the end.

That line is to test if the member is in document or not. It will throw an error if the member is not in document or if that same member is in document for more than one time, resulting in test failure AFAIK

I would also add toBeVisible in my next commit so that it assures that component is rendering member


transformResponse: (response: TasksResponseType) => {
return {
tasks: response.tasks?.sort(
tasks: [...response.tasks].sort(
Copy link
Contributor

Choose a reason for hiding this comment

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

why made this change?

Copy link
Author

Choose a reason for hiding this comment

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

why made this change?

The previous code might mutate the response, so i created a new array and pass the required changes without mutating the response

message: 'Task cannot be assigned to users with active or OOO status',
};

export function taskAssigned(Id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is a constants file, it should have this function

Copy link
Contributor

@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.

good work on the tests, left few comments, please address them

@WYGIN WYGIN requested a review from bhtibrewal July 20, 2023 11:10
@prakashchoudhary07
Copy link
Contributor

Tests are failing please look into it

@sahsisunny sahsisunny closed this Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants