tpunt / php-internals

The server application for:
https://phpinternals.net
2 stars 0 forks source link

API for versioning/reviewing changes #2

Open tpunt opened 7 years ago

tpunt commented 7 years ago

I’m unsure on how to expose the API for reviews. Do you have any thoughts? Currently, the server app enables for:

Reviewed or direct changes are already working via either the user’s privilege level (1 = must review, 2 or 3 = optional review, specified with the review query string parameter).

Focusing on deletion for now: How should I expose whether a delete is done softly or hardly? It could be via the query string again, so mode=soft as the default, and mode=hard for when an item that is already in the recycle bin needs to be deleted (for DELETE requests only).

This also brings in other things, such as:

And that's only for the deleting categories. There also needs to be a similar API for inserting and updating categories too. Updating will be possibly a little more complex, given that multiple patches for review may be given for a category, and that will require merging... The insertion API for categories should be the simplest of the three operations.

All of the above then needs to be done for symbols, and eventually for articles...

So what are your thoughts on this? We need to very clearly specify how this API is going to look, and I'd rather get a solid version of it down now so that I have a clear vision for what I'm actually doing.

tpunt commented 7 years ago

Putting the potential solutions above into the API design:

Guests (0) GET api/categories

Unverified users (1) POST api/categories PATCH api/categories/{} DELETE api/categories/{}

Changes always require reviewing

Verified users (2) GET api/categories (?status=delete_request) POST api/categories (?revert=delete_request) PATCH api/categories/{} (?review=1, defaults to 0) DELETE api/categories/{} (?review=1, defaults to 0 (soft delete, 1 being a delete request))

Admins (3) GET api/categories (?status=deleted) POST api/categories (?revert=delete) DELETE api/categories/{} (?mode=hard)

liammann commented 7 years ago

Category Insert How about sub resource for revisions and work that in to the permissions

maybe have a /api/bin resource to find all the soft deleted…

I’m not sure how todo it to be honest but you’ll probably want some sort of version control as well

Review ?review=1 basically a tick box that means i want this reviewed before going live. Shouldn’t this just be sent in the body?

tpunt commented 7 years ago

Hmm... So what you're saying is that the {{url}} would be a shortcut that points to the last {{url}}/revisions/{{version}} resource? That is, only if the last {{version}} has been approved (i.e. created by groups 2 or 3 without reviewing, or has been reviewed). Your URI structure definitely seems the most logical to use when the ability to view all revisions gets added.

For now though, I'm more unsure on how to cleanly specify which semantics an action should have. For example, should performing a DELETE api/categories/{{cat}} by an admin be a soft delete or a hard delete? The only way I can really see how to specify this is via an additional query string parameter (mode).

With regards to viewing deleted categories, I was thinking of performing a simple GET api/categories?status=deleted, where only admins would have the privilege to use the status parameter. There would therefore be no need to put the category into a special /bin collection resource, which would really only increase API complexity.

It's not exactly simple, but I'm edging towards my second comment on this issue about using the query string to distinguish between the handling of many of these actions. What are your thoughts on the names and settings chosen for these?

Regarding adding the review parameter to the request body as opposed to having it in the query string of the URI, I'm hesitant to do this. It feels like the request body containing a resource is being polluted by settings that are not apart of the resource itself. I couldn't find much information on this, but I didn't come across any material that said the query string parameter should not subjugate the semantics of a POST (or PATCH) action. Given that the query string is frequently used to manipulate an action on a resource (such as filtering on GET), I think it's best to keep these action dictations out of the way in the query string. That way, segregation can be kept between a resource's content being submitted (in the request body), and its extraneous specifics on how the server must handle the action (in the URI's query string).

liammann commented 7 years ago

Im fine with all the names you've picked, but i'ld probably add a pending status....

Verified users (2) GET api/categories (?status=delete_request | Pending? ) POST api/categories (?revert=delete_request) PATCH api/categories/{} (?review=1, defaults to 0) DELETE api/categories/{} (?review=1, defaults to 0 (soft delete, 1 being a delete request))

Would there be a way of getting back all my requests? Will there be communication between the author and reviewer?

Admins (3) GET api/categories (?status=deleted | Pending? ) POST api/categories (?revert=delete) DELETE api/categories/{} (?mode=hard)

tpunt commented 7 years ago

Ah yes, I will add a pending parameter for viewing all patches currently requiring review.

I don't fully understand your bolded questions. Do you mean whether a user will be able see if their patch has been applied or rejected (and how)? And if it has been rejected, whether the rejector it can say a message as to why it was rejected? As well as the possibility for the patch creator to reply to rejection (so basically a conversation between rejector and patch creator)?

liammann commented 7 years ago

Yeah basically if my patch is rejected I would like to be notified of why and then maybe I can improve it and resubmit. Otherwise I might feel like I'm wasting my time.

This is probably another feature though but it seems connected

tpunt commented 7 years ago

Yeah, I completely agree that we should have it, but for now I don't want to do this just yet, given the amount of other things already needing to be done. I'll get on with the above for now anyway and notify you as I complete parts, so that you can integrate it on your end.

tpunt commented 7 years ago

I've decided to remove the status parameter (with values delete_request and pending) in favour for a patches parameter. For now, this can have the value all, will be applied to GET requests on /api/categories, and can be used by verified users (2) and admins (3). This way, it can be extended to filter out specific patches by patches=delete (or update or insert).