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

Add policy author spec #3

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from
Open

Add policy author spec #3

wants to merge 6 commits into from

Conversation

janedotx
Copy link

@janedotx janedotx commented Apr 2, 2020

Explain pull request

This adds the Policy Author spec and removes the Geography related stuff in Policy, which is necessary now that Geographies will be covered in their own specs.

Is this a breaking change

None of the changes to Policy made here should affect any current implementations.

.gitignore Outdated
.venv/
*.swp

Choose a reason for hiding this comment

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

Shouldn't be modifying the .gitignore

Parameters:
| Name | Type | R/O | Description |
| ------------ | --------- | --- | ---------------------------------------------- |
| `id` | UUID | Optional | If provided, returns one policy object with the matching UUID; default is to return all policy objects. |

Choose a reason for hiding this comment

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

This seems wrong, it should definitely be a policy_id parameter (which is what it is in the reference impl)


Payload: a new Policy object, without a `policy_id`

; a failure explanation on failure

Choose a reason for hiding this comment

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

Typo?

- 401 - Unauthorized (if any auth issue)
- 500 - Server error (hopefully doesn’t happen)

### PUT /policies/{id}

Choose a reason for hiding this comment

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

Should be /{policy_id}

- 409 - conflict (if immutable)
- 500 - server error

### POST /policies/{id}/publish

Choose a reason for hiding this comment

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

/policies/{policy_id}/publish

- 500 - server error


### GET /policies/{id}/meta

Choose a reason for hiding this comment

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

/policies/{policy_id}/meta

- 500 - server error


### PUT /policies/{id}/meta

Choose a reason for hiding this comment

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

/policies/{policy_id}/meta

policy/README.md Outdated
@@ -22,6 +22,12 @@ The goal of this specification is to enable Agencies to create, revise, and publ

The machine-readable format allows Providers to obtain policies and compute compliance where it can be determined entirely by data obtained internally.

Policies will typically be linked to one or more associated geographies. Geography descriptions (e.g. geofences or lists of street segments) must also be maintained by the Agency indefinitely. Policies without specific geographies (global policies) are assumed to apply to the entire service area managed by the Agency.

Choose a reason for hiding this comment

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

Shouldn't mention service areas, no longer a thing. Perhaps better to say something like "assumed to apply to all jurisdictions managed by the Agency".

policy/README.md Outdated
@@ -22,6 +22,12 @@ The goal of this specification is to enable Agencies to create, revise, and publ

The machine-readable format allows Providers to obtain policies and compute compliance where it can be determined entirely by data obtained internally.

Policies will typically be linked to one or more associated geographies. Geography descriptions (e.g. geofences or lists of street segments) must also be maintained by the Agency indefinitely. Policies without specific geographies (global policies) are assumed to apply to the entire service area managed by the Agency.

Geographical data will be stored as immutable GeoJSON, referenced by UUID. See the Geography and Geography Author specs for information on the Geography schema, and how Agencies are expected to create and maintain Geographies and serve them to Providers. In a future revision of Agency, we will deprecate the existing `/service_areas` endpoint. `/service_areas` currently only handles GeoJSON MultiPolygon and Polygon objects, and Policy documents might prefer Points for locations such as drop-zones. Policy may be used for a variety of enforcement actions, so it's important for the Agency to persist and keep immutable both Policy and Geography data.

Choose a reason for hiding this comment

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

GeoJSON FeatureCollection instead of just GeoJSON.

/service_areas has already been deprecated.

@avatarneil
Copy link

Sidebar to note -- even after these are approved you shouldn't merge to dev, we want to keep them as isolated branches to cut distinct PRs into OMF

@janedotx janedotx dismissed avatarneil’s stale review April 28, 2020 00:29

Changes implemented.

Response codes:

- 201 - Created. Returns: the Policy object on success, including a `policy_id` and a `version` indicating the current API version.
- 400 - Policy does not conform to schema

Choose a reason for hiding this comment

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

We can also send a 409 upon a conflict

Choose a reason for hiding this comment

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

Which I suppose doesn't make sense considering we auto-fill a policy_id, I'll remove that error handling

- 500 - server error


### GET /policies/{policy_id}/meta

Choose a reason for hiding this comment

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

Should also have a 400 bad params error

- 200 - success
- 401 - unauthorized
- 404 - not found
- 409 - conflict (if already published)

Choose a reason for hiding this comment

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

Should add 424 for a missing dependency

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.

2 participants