The ability to mark a task as completed from the frontend needs to be developed

seidior commented 1 year ago

Expected behavior

The last remaining big feature of the programs/activities component is the ability to mark activities (classified as "assignments") as completed and to have a visual indicator of which ones have been completed.

Actual behavior

Right now in our Dialog component, it shows all of the details of the activity except its completion status, and there is no way to mark a task as completed.

Details and resources

Minimum requirements for resolving this issue:

When you click on a task and the Dialog element is presented, if the event is of type "assignment", the webapp should show the user's completion status (maybe with a checkbox?) in the Dialog.
Clicking either the "mark completed" button or clicking a checkbox will send an API request to mark the task as completed.
- If it's implemented as a button, there should be a different button that will now show to mark the task as "not completed", in case marking it completed was in error.
- If it's implemented as a checkbox, unchecking the checkbox should send an API request to mark the task as not completed.

Stretch goals: (not required, but nice to have)

There should be a visual indicator in the calendar for which activities are assignments and are either complete or not complete
The button/checkbox to mark a task as complete should have a visible status to it to show the different states of the API request (like spinning or other animation), and the "completion" state should only be shown if the API request succeeds
If we're unable to mark a task as completed, then there should be a visible error message displayed to the user so that they can act on it.

Reach goals: (not in scope, but if they are included, that'd be rad!)

There should be a view that shows us a list of the activities that are assignments, their due dates, and their completion status. Think kind of like a to-do list view.

Checklist

[x] This issue is linked to the appropriate project.
[x] This issue has appropriate labels.

wendyyng commented 1 year ago

Just an update, I tried to add the checkbox to the dialog.

wendyyng commented 1 year ago

Update: I modified the CalendarEvent type to include activityType. Now the activity dialog would show the checkbox if the activityType is 'assignment'.

wendyyng commented 1 year ago

Update: I tried to create an updateApiData file since useApiData can only handle get request and other parts of the app also don't allow updating the existing data. I looked at Ruthie's issue #455 and include the activityStatus, programId and activityId in the path for the PATCH request:

fetch(`${process.env.REACT_APP_API_ORIGIN}/programs/activityStatus/${programId}/${activityId}`, {
        method: 'PATCH',
        credentials: sendCredentials ? 'include' : 'omit',
        signal,
      })

wendyyng commented 1 year ago

I referred to the CreateOrUpdateCompetencyForm to add the PATCH request to the ProgramActivityDialog. We can either use a button or checkbox, and to me buttons are a little bit easier to work with, since it'd be similar to like and unlike buttons.

But I'll see if we can continue to use checkbox. I think it probably needs to use 'onChange'.

wendyyng commented 1 year ago

I changed the UI of the dialog based on the discussion today. But I think I'll wait #453 to be merged into main before making more commits so that the merge conflict won't be too complicated. Also the activity type is now included in the dialog.

wendyyng commented 1 year ago

Instead of making another helper function, we can use MUI's text transformation to capitalize text.

        <DialogContentText id="alert-dialog-description" sx={{textTransform: 'capitalize'}}>
          <b>Activity Type:</b> {contents.activityType}
        </DialogContentText>

wendyyng commented 1 year ago

Added the buttons after merging from main:

wendyyng commented 1 year ago

Adding radio button to the calendar:

 { event.event.activityType === decodeHTML('assignment') && (
          <RadioButtonUncheckedIcon fontSize='inherit' sx={{ pt: 0.5}}/>
        )
        }

wendyyng commented 1 year ago

Using different color for assignments:

seidior commented 1 year ago

I like it! Great work so far as always, @wendyyng.

Any particular reason the "Mark Complete"/"Mark Incomplete" button is better in the dialog content section rather than the left-side of the dialog actions section? No worries if you all discussed that already or if you think it's better there. I'm just curious as to the choice, that's all.

If you prefer the dialog content section, maybe we can move it to a table row like the rest of the content?

Also, it appears from your last update that adding the icon into the activity in week view is pushing over the width of the days in the all-day events section, so by the time Saturday hits, the all day section is pretty misaligned with the section below it. Not sure if that's just a WIP fragment or an uncaught error.

Heads up also, you should be getting a route to make your life easier so you can retrieve the statuses of all assignments at once instead of one by one. And I cleaned up some of my repeated styled-components values; sorry for leaving that kind of messy.

Two small other notes:

again, let's make sure that we use  instead of  to embolden text with emphasis, as seen in this comment from 3 days ago
you don't need to decodeHTML() for any values that can't be input by the user or don't have special characters. So, we'd want it on the description for sure, but we don't need to do it for an activity type, since those are hard-coded in the database and shouldn't have any special chars.

wendyyng commented 1 year ago

Updates on the Dialog's UI: We got two options!

Replacing the close button with the close icon on the top right and moving the Mark Complete button to the bottom right:
Moving the Mark Complete button to the left of the Dialog Section

wendyyng commented 1 year ago

I like it! Great work so far as always, @wendyyng.

Any particular reason the "Mark Complete"/"Mark Incomplete" button is better in the dialog content section rather than the left-side of the dialog actions section? No worries if you all discussed that already or if you think it's better there. I'm just curious as to the choice, that's all.

If you prefer the dialog content section, maybe we can move it to a table row like the rest of the content?

Also, it appears from your last update that adding the icon into the activity in week view is pushing over the width of the days in the all-day events section, so by the time Saturday hits, the all day section is pretty misaligned with the section below it. Not sure if that's just a WIP fragment or an uncaught error.

Heads up also, you should be getting a route to make your life easier so you can retrieve the statuses of all assignments at once instead of one by one. And I cleaned up some of my repeated styled-components values; sorry for leaving that kind of messy.

Two small other notes:

again, let's make sure that we use  instead of  to embolden text with emphasis, as seen in this comment from 3 days ago

you don't need to decodeHTML() for any values that can't be input by the user or don't have special characters. So, we'd want it on the description for sure, but we don't need to do it for an activity type, since those are hard-coded in the database and shouldn't have any special chars.

Adding the icon into the activity in week view is pushing over the width of the days in the all-day events section:

You're right! And based on the discussion today, this will be out of scope of this PR. So I won't make commits for this.

Use  instead of  to embolden text with emphasis:

Thanks for the reminder! For now we're using fontWeight: 'bold' instead though so no  or  is being used.

Don't need to decodeHTML() for any values that can't be input by the user or don't have special characters. So, we'd want it on the description for sure, but we don't need to do it for an activity type:

It has been removed! :)

veranika-karpava commented 1 year ago

Great work, Wendy. Both options look wonderful, but I prefer the first one.

SanazDn commented 1 year ago

Great job! I prefer first one as well.

daveytech commented 1 year ago

Thanks for working on those screenshots. I agree with the others, the first one is nice and clean! My vote is that one!

wendyyng commented 1 year ago

12/6: Working on the GET and POST for activity status

// Get Request for activity completsion status
  // programsRouter.get('/activityStatus/:programId/:activityId')
  const {
    data: activityCompletionStatus,
    error,
    isLoading,
  } = useApiData<ActivityCompletionStatus>({
    deps: [completed],
    path: `/programs/activityStatus/${contents.programId}/${contents.curriculumActivityId}`,
    sendCredentials: true,
  });

 // Post Request for updating activity completion status
  // programsRouter.post('/activityStatus/:programId/:activityId')
  // Using onClick for UI demo but onSubmit should be used in future development
  const onClick: FormEventHandler = event => {
    event.preventDefault();
    setCompleted(!completed);
    // fetch(
    //   `${process.env.REACT_APP_API_ORIGIN}/programs/activityStatus/${programId}/${activityId}`,
    //   {
    //     method: 'POST',
    //     credentials: 'include',
    //     headers: { 'Content-Type': 'application/json' },
    //   }
    // )
    //   .then(res => res.json())
    //   .then(({ data, error }) => {
    //     setCompleted(!completed);
    //     if (error) {
    //       setMarkCompletedError(error);
    //     }
    //     if (data) {
    //       setIsSuccessMessageVisible(true);
    //     }
    //   })
    //   .catch(error => {
    //     setCompleted(completed);
    //     setMarkCompletedError(error);
    //   });
  };

wendyyng commented 1 year ago

12/7 Continued to work on the GET & POST requests:

  const [completed, setCompleted] = useState(false);
  // const [markCompletedError, setMarkCompletedError] = useState(null);
  // const [isSuccessMessageVisible, setIsSuccessMessageVisible] = useState(false);

  // Get Request for activity completsion status
  // programsRouter.get('/activityStatus/:programId/:activityId')
  // return {activityId:1, participantActivityId: 1, completed: true}
  const {
    data: activityCompletionStatus,
    error,
    isLoading,
  } = useApiData<ActivityCompletionStatus>({
    deps: [completed],
    path: `/programs/activityStatus/${contents.programId}/${contents.curriculumActivityId}`,
    sendCredentials: true,
  });

  // Post Request for updating activity completion status
  // programsRouter.post('/activityStatus/:programId/:activityId')
  // request body shoud look like: {"completed": true}
  // Using onClick for UI demo but onSubmit should be used in future development
  const onClick: FormEventHandler = event => {
    event.preventDefault();
    setCompleted(!completed);
    // fetch(
    //   `${process.env.REACT_APP_API_ORIGIN}/programs/activityStatus/${contents.programId}/${contents.curriculumActivityId}`,
    //   {
    //     method: 'POST',
    //     credentials: 'include',
    //     headers: { 'Content-Type': 'application/json' },
    //     body: JSON.stringify({
    //     completed: !completed
    // }),
    //   },
    // )
    //   .then(res => res.json())
    //   .then(({ data, error }) => {
    //     setCompleted(!completed);
    //     if (error) {
    //       setMarkCompletedError(error);
    //     }
    //     if (data) {
    //       setIsSuccessMessageVisible(true);
    //     }
    //   })
    //   .catch(error => {
    //     setCompleted(completed);
    //     setMarkCompletedError(error);
    //   });
  };

seidior commented 1 year ago

Updates on the latest from this issue:

Phoebe and Wendy worked late into the night last night to make sure that this code was working given the changes to the backend code. We worked through some of the issues with React state management to be able to demonstrate working functionality, but the code isn't conformant to guidelines at the moment.

Next for this ticket:

I'd love to find a way to useApiData instead of the fetch() call for the getter. The problem right now is that the getter needs to be called conditionally, which useApiData doesn't currently support.
I'd love to move the PUT fetch() logic somewhere else like a helper function so that other code can use it, then add appropriate tests for that.
There's currently minimal to no error handling if the getter or setter fail for any reason, so I'd love to see some code to handle that.
Extraneous code comments need to be cleaned up and some refactoring needs to be done to get code to conform to guidelines

OpenTree-Education / rhizone-lms