Skip to content

Conversation

@Saitharun279
Copy link

@Saitharun279 Saitharun279 commented Jan 16, 2025

Date: 17 January, 2025

Developer Name: Saitharun B


Issue Ticket Number

Description

Added code changes to let task owner do the following:

  • Task Status Updation
  • View purpose of the task

Documentation Updated?

  • Yes
  • No

Under Feature Flag

  • Yes
  • No

Database Changes

  • Yes
  • No

Breaking Changes

  • Yes
  • No

Development Tested?

  • Yes
  • No

Screenshots

Web
local_testing.mov
mobile
local.testing.moble.mov

Test Coverage

Screenshot Screenshot 2025-01-17 at 2 59 16 AM Screenshot 2025-01-17 at 2 57 44 AM Screenshot 2025-01-17 at 2 57 59 AM Screenshot 2025-01-17 at 2 58 08 AM Screenshot 2025-01-17 at 2 58 18 AM

Additional Notes

design doc link

@vercel
Copy link

vercel bot commented Jan 16, 2025

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

A member of the Team first needs to authorize it.

@Saitharun279 Saitharun279 changed the title Feature/issue#1263 purpose status Add missing features to task card issue#1263-1 ( purpose & status ) Jan 16, 2025
@Saitharun279 Saitharun279 changed the title Add missing features to task card issue#1263-1 ( purpose & status ) Add missing features to task card issue#1263 -1 ( purpose & status ) Jan 16, 2025
Copy link
Contributor

@tejaskh3 tejaskh3 left a comment

Choose a reason for hiding this comment

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

  • Please update this branch with develop.
  • please pass me the link to the design you have updated.
  • and if there is any design doc created for this task, please attach that to this PR details.

@Saitharun279
Copy link
Author

Saitharun279 commented Jan 17, 2025

  1. Updated this branch with develop.
  2. I didn't make any design made for this, the task card here is being updated with reference from task card in my site.
    Reference images have been included in the design doc.
  3. Attached design doc link.

Copy link
Contributor

@AnujChhikara AnujChhikara left a comment

Choose a reason for hiding this comment

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

Hello @Saitharun279, could you please check if we can use predefined color variables from variables.scss instead of adding random colors?

Purpose:{' '}
</b>
<span className={styles.cardPurposeText}>
{editedTaskDetails.purpose}
Copy link
Contributor

Choose a reason for hiding this comment

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

do we have purpose field when we create the TCR from status site?

Copy link
Author

Choose a reason for hiding this comment

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

I just checked, it is not present in the TCR.
I think it needs to be added.


.cardPurposeAndStatusFont {
font-size: 1.1rem;
color: #555;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use a predefined color from variables.scss here?

Copy link
Author

Choose a reason for hiding this comment

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

All the colors that I used are not present in variables.css. I have taken them from my site.
should i add them in variables.scss ?

@AnujChhikara
Copy link
Contributor

  • Could you write tests for your changes?
  • Could you please include the mobile view in the screenshot as well?

@Saitharun279
Copy link
Author

Saitharun279 commented Jan 18, 2025

  • Could you write tests for your changes?
  • Added tests.
  • Could you please include the mobile view in the screenshot as well?

Attached mobile video

}

.taskStatusUpdate {
border: 1px solid #000000b3;
Copy link
Contributor

Choose a reason for hiding this comment

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

  • are there existing css variables we can use instead of hardcoding the color?

Copy link
Author

Choose a reason for hiding this comment

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

Didn't find any existing css variables for this one.

};
const isEditable = shouldEdit && isUserAuthorized && isEditMode;
const isSelfTask = editedTaskDetails.assignee === data?.username;
const verifiedTask =
Copy link
Contributor

Choose a reason for hiding this comment

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

  • should this be named isTaskCompleted as we're doing a check on the percentage this task has be completed?

Copy link
Author

Choose a reason for hiding this comment

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

  1. The dropdown to update task staus should only be shown for non verified tasks, this variable verifiedTask is to check if task status is verified.
  2. The check for 100 percent completion can be removed, as the status updation API allows update to Verified when percentage is 100.
    I will remove this percentage check in next commit.

I think the variable can be renamed to isVerifiedTask.

Copy link
Author

Choose a reason for hiding this comment

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

Removed 100 percentage check and modified variable name to isVerifiedTask

const [updateTask] = useUpdateTaskMutation();
const [updateSelfTask] = useUpdateSelfTaskMutation();

const onChangeUpdateTaskStatus = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

  • can we update this function to use try catch to make this cleaner and easier to update? example:
const onChangeUpdateTaskStatus = async ({
        newStatus,
        newProgress,
    }: taskStatusUpdateHandleProp) => {
        setSaveStatus(PENDING);
        const payload: { status: string; percentCompleted?: number } = {
            status: newStatus,
        };

        if (newProgress !== undefined) {
            payload.percentCompleted = newProgress;
        }

        setEditedTaskDetails((prev: CardTaskDetails) => ({
            ...prev,
            ...payload,
        }));

        try{
            if (isDevMode && isSelfTask){
                await updateSelfTask({ id: task.id, task: payload })
            } else {
                await updateTask({ id: task.id, task: payload });
            }
        } catch(error){
            setSaveStatus(ERROR_STATUS);
        } finally {
            setTimeout(() => { 
                setSaveStatus('');
            }, 3000);
        }
    };
  • should we call setEditedTaskDetails after the API call is successful preventing any inconsistent states?
  • can we remove the setTimeout from the finally block?

Copy link
Author

Choose a reason for hiding this comment

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

Will update in next commit.

But not sure about removing setTimeout, will check and update that as well.

Copy link
Author

Choose a reason for hiding this comment

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

Done

@yesyash
Copy link
Contributor

yesyash commented Jan 18, 2025

@Saitharun279 Saitharun279 changed the title Add missing features to task card issue#1263 -1 ( purpose & status ) Adding 2 of the missing features ( purpose & status ) to task card issue#1263 Jan 18, 2025
@Saitharun279 Saitharun279 changed the title Adding 2 of the missing features ( purpose & status ) to task card issue#1263 Adding 2 of the missing features ( purpose & status ) to task card Jan 18, 2025
@Saitharun279 Saitharun279 changed the title Adding 2 of the missing features ( purpose & status ) to task card feat(1263): Add purpose & status to task card, based on task card in website-my Jan 18, 2025
@Saitharun279 Saitharun279 changed the title feat(1263): Add purpose & status to task card, based on task card in website-my feat(1263): Add purpose & status to task card, based on task card in website-my #1263 Jan 18, 2025
@Saitharun279 Saitharun279 changed the title feat(1263): Add purpose & status to task card, based on task card in website-my #1263 feat(1263): Add purpose & status to task card, based on task card in website-my Jan 18, 2025
Comment on lines +64 to +68
try {
if (isDevMode && isSelfTask) {
await updateSelfTask({ id: task.id, task: payload }).unwrap();
}
toast(SUCCESS, STATUS_UPDATE_SUCCESSFUL);
Copy link

Choose a reason for hiding this comment

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

The success toast is displayed unconditionally, regardless of whether updateSelfTask was executed or successful. Consider moving the toast inside the if block after the await call to ensure it only appears when the operation actually succeeds:

try {
    if (isDevMode && isSelfTask) {
        await updateSelfTask({ id: task.id, task: payload }).unwrap();
        toast(SUCCESS, STATUS_UPDATE_SUCCESSFUL);
    }
}

This way, the success message will only be shown when the task update operation is both attempted and completed successfully.

Suggested change
try {
if (isDevMode && isSelfTask) {
await updateSelfTask({ id: task.id, task: payload }).unwrap();
}
toast(SUCCESS, STATUS_UPDATE_SUCCESSFUL);
try {
if (isDevMode && isSelfTask) {
await updateSelfTask({ id: task.id, task: payload }).unwrap();
toast(SUCCESS, STATUS_UPDATE_SUCCESSFUL);
}

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

Comment on lines +179 to +198
it('change task status from BLOCKED to IN_PROGRESS when and isDevMode are true', async () => {
const setEditedTaskDetails = jest.fn();

renderWithRouter(
<Provider store={store()}>
<TaskStatusEditMode
task={BLOCKED_TASK}
setEditedTaskDetails={setEditedTaskDetails}
isDevMode={true}
isSelfTask={true}
/>
</Provider>
);

const statusSelect = screen.getByLabelText('Status:');
expect(statusSelect).toHaveValue('BLOCKED');

fireEvent.change(statusSelect, { target: { value: 'IN_PROGRESS' } });
expect(statusSelect).toHaveValue('IN_PROGRESS');
});
Copy link

Choose a reason for hiding this comment

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

This test verifies the UI state changes but doesn't confirm the actual API call functionality. Consider adding an assertion to verify that updateSelfTaskMutation is called with the correct parameters:

expect(updateSelfTaskSpy).toHaveBeenCalledWith({ 
  id: BLOCKED_TASK.id, 
  task: expect.objectContaining({ status: 'IN_PROGRESS' }) 
});

This would ensure the status update is properly propagated to the API layer, not just reflected in the UI component.

Suggested change
it('change task status from BLOCKED to IN_PROGRESS when and isDevMode are true', async () => {
const setEditedTaskDetails = jest.fn();
renderWithRouter(
<Provider store={store()}>
<TaskStatusEditMode
task={BLOCKED_TASK}
setEditedTaskDetails={setEditedTaskDetails}
isDevMode={true}
isSelfTask={true}
/>
</Provider>
);
const statusSelect = screen.getByLabelText('Status:');
expect(statusSelect).toHaveValue('BLOCKED');
fireEvent.change(statusSelect, { target: { value: 'IN_PROGRESS' } });
expect(statusSelect).toHaveValue('IN_PROGRESS');
});
it('change task status from BLOCKED to IN_PROGRESS when isSelfTask and isDevMode are true', async () => {
const setEditedTaskDetails = jest.fn();
const updateSelfTaskSpy = jest.fn().mockResolvedValue({});
jest.spyOn(taskHooks, 'useUpdateSelfTaskMutation').mockReturnValue([updateSelfTaskSpy, { isLoading: false }]);
renderWithRouter(
<Provider store={store()}>
<TaskStatusEditMode
task={BLOCKED_TASK}
setEditedTaskDetails={setEditedTaskDetails}
isDevMode={true}
isSelfTask={true}
/>
</Provider>
);
const statusSelect = screen.getByLabelText('Status:');
expect(statusSelect).toHaveValue('BLOCKED');
fireEvent.change(statusSelect, { target: { value: 'IN_PROGRESS' } });
expect(statusSelect).toHaveValue('IN_PROGRESS');
expect(updateSelfTaskSpy).toHaveBeenCalledWith({
id: BLOCKED_TASK.id,
task: expect.objectContaining({ status: 'IN_PROGRESS' })
});
});

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

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.

feat : add missing features in Task Card Component

4 participants