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

Outline REST API #115

Merged
merged 7 commits into from
Oct 10, 2022
Merged
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 77 additions & 0 deletions doc/arch/adr-002-rest-api.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
# Boundary Sync REST API Design

## Context

Based on the [data model](./adr-001-data-models.md), the following REST API design is proposed to access it:

### Authentication

| Path | Method | Body | Authentication | Responses | Notes |
| --------------- | ------ | --------- | -------------- | ----------------------------------------------------- | -------------------------------------------- |
| `/auth/login/` | GET | - | Allow Any | 200 OK,<br />401 Not Authorized | Returns loging JSON if current session valid |
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't think this request returned anything other than a status code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should return at least the Role of the user, so decisions can be made in the front-end. It may also return the user's email, which can be shown in the UI as the currently logged in user. We don't have a spot for this in the UI, but most apps do, and we may well have one at some point.

| `/auth/login/` | POST | `{ ... }` | Allow Any | 200 OK,<br />400 Bad Request,<br />401 Not Authorized | Used for starting new valid session |
| `/auth/logout/` | POST | - | Logged In User | 200 OK | Used for logging out |
rajadain marked this conversation as resolved.
Show resolved Hide resolved

Also present are `/auth/password/reset/` and `/auth/password/reset/confirm/` endpoints that come from `dj_rest_auth`.

### Boundaries
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a little conflicted about the Boundary / Submission separation. In our data model, the Boundary is the top-level entity, which may have one or more Submissions through its life. But in the UI, we're calling everything Submissions, so this is getting a little confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm willing to get rid of Boundary and just treat Utilities as boundaries (== groups of submissions).

Copy link
Contributor

Choose a reason for hiding this comment

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

To flesh out that thought a little: otherwise, if we leave Boundary as a model, then I'm okay with the design in this ADR as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A utility can have multiple boundaries submitted over the years, and each boundary is a group of submissions, so we can't make that direct link. Need something in between.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused by this.

With /boundaries/{id}/submit/, is the id the submission ID or the boundary ID? If not the submission ID, then where is that data? In the body of the request? I don't think we can determine the submission ID just from the boundary ID.

But in the UI, we're calling everything Submissions, so this is getting a little confusing.

The users aren't going to see the API routes, so why not use naming that reflects the data model?

Copy link
Contributor

Choose a reason for hiding this comment

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

A utility can have multiple boundaries submitted over the years, and each boundary is a group of submissions, so we can't make that direct link. Need something in between.

Right, but I'm not aware of any designs that pay attention to years, I thought all of that was going to be handled on the backend with get_latest submissions etc. Maybe we should consider adding a year field to Boundary to make it explicit? Could be discussed in a separate thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We make them transparent to the front-end because the correct one can be determined in the back-end?

Correct.

I'm imagining that POSTing to /boundaries/ creates a Boundary and a Submission, and returns details for both. POSTING to /boundaries/{id}/submit/ updates the Submission with notes and sets its submitted_at and submitted_by.

This reveals that we need an additional endpoint for creating a second draft submission. I'll add one.

Copy link
Contributor

Choose a reason for hiding this comment

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

But only one draft can be in review at a time? (Because 0-1 reviews per boundary.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, only one active submission (in draft or submitted state) at a time.

Copy link
Contributor

@jacobtylerwalls jacobtylerwalls Oct 10, 2022

Choose a reason for hiding this comment

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

Update from scrum: not merely one draft in review at a time, but one draft at a time period. Coworkers will be collaborating on the same draft. I guess my question didn't make sense because a draft can't be in review.


| Path | Method | Body | Authentication | Responses | Notes |
| ------------------------------------------ | ------ | ------------------------ | -------------- | ---------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------ |
| `/boundaries/` | GET | - | Logged In User | 200 OK | Returns list of all boundaries the user has access to |
| `/boundaries/?utilities=1,3,5,8` | GET | - | Logged In User | 200 OK,<br />401 Not Authorized | Returns list of boundaries in the given utilities. If the user does not have access to that utility, returns a 401 |
| `/boundaries/` | POST | `{ ... }` | Contributor | 201 Created,<br />400 Bad Request,<br />401 Not Authorized | Creates a new boundary if the payload is correct and the user is authorized |
| `/boundaries/{id}/` | GET | - | Logged In User | 200 OK,<br />404 Not Found | Returns boundary details if the user has access to it, else 404s. Includes all details about the boundary. |
| `/boundaries/{id}/shape/` | PUT | `{ ... }` | Contributor | 200 OK,<br />404 Not Found | Updates the latest submission's shape |
| `/boundaries/{id}/reference-images/` | POST | `{ ... }` | Contributor | 200 OK,<br />404 Not Found | Adds a new reference image to the boundary |
rajadain marked this conversation as resolved.
Show resolved Hide resolved
rajadain marked this conversation as resolved.
Show resolved Hide resolved
| `/boundaries/{id}/submit/` | POST | `{ notes }` | Contributor | 200 OK,<br />404 Not Found | Submits the boundary |
| `/boundaries/{id}/review/` | POST | `{ notes, annotations }` | Validator | 200 OK,<br />404 Not Found | Reviews a boundary |
| `/boundaries/{id}/review/annotations/{id}` | PUT | `{ annotation }` | Validator | 200 OK,<br />404 Not Found | Updates an annotation in the latest review. Older reviews are read-only, so no need to specify review id. |
| `/boundaries/{id}/draft/` | POST | - | Contributor | 200 OK,<br />404 Not Found | Creates a new draft submission for a boundary after a review |
| `/boundaries/{id}/approve/` | POST | - | Validator | 200 OK,<br />404 Not Found | Approves a boundary |
| `/boundaries/{id}/unapprove/` | POST | - | Validator | 200 OK,<br />404 Not Found | Unapproves a boundary |

### User

| Path | Method | Body | Authentication | Responses | Notes |
| -------------------- | ------ | --------- | -------------- | -------------------------- | ---------------------------------- |
| `/user/{id}/profile` | PUT | `{ ... }` | Contributor | 200 OK,<br />404 Not Found | Update contributor contact details |

## Notes

Any thing that a Contributor or Validator can do, an Administrator can also do.

A payload Body of `-` means that there is no body required. A payload Body of `{ ... }` means that there is a required body, but is not elaborated here.

All endpoints that are marked as requiring a Logged In User will also return 401 Not Authorized if accessed without proper credentials.

When we return `404 Not Found` for objects that the User does not have permissions to, we're using a pattern like this:

```python
@permission_classes((IsAuthenticated,))
def get_boundary(request, id):
user = request.user
boundary = get_object_or_404(Boundary, id)
submission = boundary.get_latest_submission_for(user)

serializer = SubmissionSerializer(submission)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is also confusing, because the endpoint is called get_boundary but we're returning a serialized submission.

Perhaps there can be a BoundaryDetailSerializer that takes a boundary and does the get_latest_submission_for(user) within it, so we can return that here.

return Response(serializer.data)
```

where `get_latest_submission_for` is along these lines:

```python
class Boundary(models.Model):
...

def get_latest_submission_for(self, user):
if user.role == CONTRIBUTOR:
jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved
if self.utility not in user.utilities:
raise Http404

if user.role == VALIDATOR:
if self.utility.state not in user.states:
raise Http404

return self.submissions.latest('created_at')
```