OpenTree-Education / rhizone-lms

A learning management system focused on self-reflection.
https://rhi.zone
BSD 3-Clause Clear License
14 stars 7 forks source link

programsService should support updating of ParticipantActivities completion status #454

Closed phoebehala closed 1 year ago

phoebehala commented 1 year ago

Expected behaviour

There should be service functions that marks a target activity as completed once a user click the complete button on an Activity Dialog. And also having the corresponding test to make sure the function works correctly.

Actual behaviour

It currently doesn't exist.

Details and resources

This ticket associated with the other backend router ticket #455 is to provide functions for the frontend ticket (#...) to produce a feature that user can mark any activity as completed.


- If the row does not exist in the table, return false.
- If the row does exist in the table, return the value of the 'completed' attribute

tests/programsService.ts should also be updated to test these new service functions.

Checklist

phoebehala commented 1 year ago

Nov. 30 status updates:

[Quote from Spencer's proposals]

  1. Handle things like I said with these three functions you have, and to return only if there's a definite result and to throw an error when the service function is called with bad parameters, knowing that both the test file and router file will appropriately handle the error.
  2. Create helper functions that Ruthie can call before calling your main functions that help do the consistency checks, and only have her error handling being our only defense against inconsistent data.
  3. Skip consistency checks altogether and allow people to write any data to the database without throwing an error, no matter how inconsistent the data being retrieved/inserted.
phoebehala commented 1 year ago

Dec. 1 status updates:

  • service test functions
wendyyng commented 1 year ago

I made a testbutton which you add on a page to test the api. I use PATCH here but should we use POST instead?

import React, { FormEventHandler } from 'react';
import { Button, FormGroup } from '@mui/material';

export const TestButton = () => {
  let programId: number = 1;
  let activityId: number = 1;

  const onSubmit: FormEventHandler = event => {
    event.preventDefault();

    fetch(
      `${process.env.REACT_APP_API_ORIGIN}/activityStatus/${programId}/${activityId}`,
      {
        method: 'PATCH',
        credentials: 'include',
        headers: { 'Content-Type': 'application/json' },
      }
    )
      .then(res => res.json())
      .then(({ data, error }) => {
        if (error) {
          console.log(Error);
        }
        if (data) {
          console.log(data);
        }
      })
      .catch(error => {
        console.log(error);
      });
  };

  return (
    <FormGroup
      onSubmit={onSubmit}
    >
      <Button type="submit" variant="contained">
        Mark Completed
      </Button>
    </FormGroup>
  );
};
phoebehala commented 1 year ago

Dec. 2 status updates:

Working on 'mock functions' to tackle the test error

phoebehala commented 1 year ago

I made a testbutton which you add on a page to test the api. I use PATCH here but should we use POST instead?

import React, { FormEventHandler } from 'react';
import { Button, FormGroup } from '@mui/material';

export const TestButton = () => {
  let programId: number = 1;
  let activityId: number = 1;

  const onSubmit: FormEventHandler = event => {
    event.preventDefault();

    fetch(
      `${process.env.REACT_APP_API_ORIGIN}/activityStatus/${programId}/${activityId}`,
      {
        method: 'PATCH',
        credentials: 'include',
        headers: { 'Content-Type': 'application/json' },
      }
    )
      .then(res => res.json())
      .then(({ data, error }) => {
        if (error) {
          console.log(Error);
        }
        if (data) {
          console.log(data);
        }
      })
      .catch(error => {
        console.log(error);
      });
  };

  return (
    <FormGroup
      onSubmit={onSubmit}
    >
      <Button type="submit" variant="contained">
        Mark Completed
      </Button>
    </FormGroup>
  );
};

thanks again! The api route will be ${process.env.REACT_APP_API_ORIGIN}/programs/activityStatus/${programId}/${activityId} As for the request method, I remember at the beginning of this week, we talked about using post instead of put as we would create the table. I'am not sure if it's still in the case. I think we can disccuss it at the next stand up :)

seidior commented 1 year ago

This is how I think about POST vs PUT vs PATCH:

POST is typically used when creating a new resource, one that is known not to already exist in a database. This should be used for actions that are non-idempotent; that is to say, if you re-submit the same exact request, it should create an additional item in the database, to start executing a process again, or whatever you have chained to the POST request will happen an additional time.

When executed and everything is successful, the typical response is HTTP 201 with a Location: header to the newly-created resource (if applicable).

For example:

POST /customer with body {"first_name": "Spencer"} would result in HTTP 201 with Location: /customer/839475938 POST /customer with the same body message would result in HTTP 201 with Location: /customer/839475939

PUT is defined in the HTTP specification as an idempotent action. So, if I submit the same request to the server, it should have the same action every single time. It's typically used to update a whole record, but sometimes it's used to upsert a record (update if exists, otherwise insert) as long as you can upsert idempotently (inserting/updating twice will not result in duplicate records).

It's usually executed on a unique URL that points to the resource being updated, such as

PUT /customer/839475938

However, in its upsert form, it's executed on a non-unique URL (such as /customers).

When executed and everything is successful for an update, either HTTP 200 "OK" or HTTP 204 "No Content" are acceptable responses. For upsert requests where an insert was performed, an HTTP 201 response would be used.

For example:

PUT /customer/839475938 with body {"first_name": "Davey"} would result in HTTP 204. PUT /customer/839475938 again with body {"first_name": "Davey"} would result in HTTP 204. GET /customer/839475938 would return HTTP 200 with body {"first_name": "Davey"}

PUT /customer with body {"id": 839475939, "first_name": "Davey"} would result in HTTP 204. PUT /customer with body {"id": 839475940, "first_name": "Davey"} would result in HTTP 201.

PATCH is like PUT, but it's typically only used to update part of a resource, such as if I was going to update a customer's first name, nickname, or anything else but to leave the rest of the record in place. Again, it's typically executed on a unique URL that points to the resource being updated; however, unlike PUT, it's not used for upsert actions, only updates.

tl;dr:

Links:

seidior commented 1 year ago

Hello! I've brought the branch up to date with the latest from main. I've also added in stubs in the testing file for the additional service file functions that will need to be tested.

I've added a new stub function in the service file to support the proposed feature where we get a list of assignments and their completion status. This will be the trickiest to write, since the list of program activities and the list of assignment completion statuses in the table won't be the same, and you'll have to construct a union of the two sets of values. I have no doubt that you'll be able to figure it out though given how far you've gotten on this so far!

Also, please remember that code comments—like any other documentation that we generate as programmers—should be held to writing standards. Please try to double-check your code comments for misspellings, typos, and any other common errors before committing your changes!

Thank you.

seidior commented 1 year ago

As per our discussion this morning, I've removed the stubs I added before to restrict scope for the backend tickets. Valuable feedback, thanks!

phoebehala commented 1 year ago

Dec. 5 status updates:

daveytech commented 1 year ago

An update on what should be returned. For getParticipantActivityCompletion and setParticipantActivityCompletion we should be returning objects. We don't want to return a Boolean as they are harder for the router to handle. And we would want to send the updated completed status after updating so the front end can confirm the update and render the change in realtime.

Both functions should return:

{activityId:1, participantActivityId: 1, completed: true}

In the future for our extended feature we would not need to send activityId.

We can review this during Wed 7th StandUp for all of us to gain alignment.

phoebehala commented 1 year ago

An update on what should be returned. For getParticipantActivityCompletion and setParticipantActivityCompletion we should be returning objects. We don't want to return a Boolean as they are harder for the router to handle. And we would want to send the updated completed status after updating so the front end can confirm the update and render the change in realtime.

Both functions should return:

{activityId:1, participantActivityId: 1, completed: true}

In the future for our extended feature we would not need to send activityId.

We can review this during Wed 7th StandUp for all of us to gain alignment.

Dec 6th status updates: