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

Outline REST API #115

merged 7 commits into from
Oct 10, 2022

Conversation

rajadain
Copy link
Contributor

@rajadain rajadain commented Oct 10, 2022

Overview

Adds an outline of the REST API so we have common target to hit.

Closes #116

Testing Instructions

  • Read through the document, ensure it makes sense

Checklist

  • fixup! commits have been squashed
  • CHANGELOG.md updated with summary of features or fixes, following Keep a Changelog guidelines
  • README.md updated if necessary to reflect the changes
  • CI passes after rebase

Copy link
Contributor Author

@rajadain rajadain left a comment

Choose a reason for hiding this comment

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

Left some comments. Discussion encouraged.

| `/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 |

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

| `/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 users has access to it, else 404s. Includes all details about the boundary. |
Copy link
Contributor Author

@rajadain rajadain Oct 10, 2022

Choose a reason for hiding this comment

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

When GETing, this would include:

  • The shape
  • List of reference layers reference images
  • Current status (Draft / Submitted / etc)
  • Current Review Annotations
  • Activity Log
  • Created At, Created By, etc

Copy link
Contributor

Choose a reason for hiding this comment

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

List of reference layers

For my own understanding: this refers to reference images?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Yes I mean reference images.

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.

doc/arch/adr-002-rest-api.md Show resolved Hide resolved
@rajadain rajadain added the Task 3.3 Django back-end label Oct 10, 2022

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

doc/arch/adr-002-rest-api.md Show resolved Hide resolved
doc/arch/adr-002-rest-api.md Outdated Show resolved Hide resolved
| `/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 users has access to it, else 404s. Includes all details about the boundary. |
Copy link
Contributor

Choose a reason for hiding this comment

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

List of reference layers

For my own understanding: this refers to reference images?

doc/arch/adr-002-rest-api.md Outdated Show resolved Hide resolved
doc/arch/adr-002-rest-api.md Outdated Show resolved Hide resolved
doc/arch/adr-002-rest-api.md Outdated Show resolved Hide resolved
doc/arch/adr-002-rest-api.md Outdated Show resolved Hide resolved
@jacobtylerwalls jacobtylerwalls mentioned this pull request Oct 10, 2022
4 tasks
@rajadain rajadain marked this pull request as ready for review October 10, 2022 13:22
@mstone121 mstone121 mentioned this pull request Oct 10, 2022
4 tasks
Copy link
Contributor

@rachelekm rachelekm left a comment

Choose a reason for hiding this comment

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

I don't have any other comments that haven't already been discussed and clarified—thank you for writing this up!

skip-ci: true
@rajadain
Copy link
Contributor Author

Thanks for all the reviews!

Eventually we'll have a Swagger API for this, but until then we should try to keep this ADR up to date whenever we add new endpoints.

@rajadain rajadain merged commit 100e8d0 into develop Oct 10, 2022
@rajadain rajadain deleted the tt/rest-api-design branch October 10, 2022 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Task 3.3 Django back-end
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Design REST API
4 participants