Below are manual tests performed by pulling down the PR, running the TPEN Services API, and using Talend API as a means to use the TPEN Services API from the perspective of an application. Note that only functionality interpreted as incorrect is reported, the code itself was not looked at. Essentially, this is a beta tester's report.
As a general note, I consider these things a barrier to PR approval and they make me unsure of exactly what to review and how to review it, and what things to 'just take care of' as I review it.
auth
The 401s returned from this class are HTML formatted error stacks. Other error response bodies are simplified messages, shouldn't these be as well? I also note here that this text/html response is typical. Worth keeping in mind when looking at comments on error response bodies below.
/project endpoint
GET /project/123 results in a 200 with a JSON response. The json has a 'message' key that reports "No TPEN3 project with ID '123' found". The best RESTful practice for this is to use a response code of 404. A response body is not strictly necessary, and we should only be returning JSON if it is necessary. The response body here would be much better as a string, or empty.
GET /project/ as a comparison to what is in the previous bullet point.
POST /project/create/ with a 'valid' Project object as the request body. Results in a 200 with a JSON response. My attempt to mess up @type and creator were caught, but I successfully submitted the incorrect @context. That @context value should work exactly like @type; it is a constant value that can't be overruled and is not required as part of the JSON body. It also allows me to save foreign keys into the database, such as {"hack": "db.tpen.drop()"}. The best RESTful practice for creating an object is to use a response code of 201 with a Location header that has id of the object created. A body is not strictly necessary, but handing back the JSON that is the created object is desired here and working correctly.
DELETE /project/create which we expect to error out as a 405. It does, however the response is JSON with a message. We should not use JSON unless there is a purpose to. The response body would be better as a string, or empty.
PATCH project/import?createFrom=url which we expect to error out as a 405. It does, however the response is JSON with a message. We should not use JSON unless there is a purpose to. The response body would be better as a string, or empty.
/manifest endpoint
Note that this endpoint is outside the scope of this PR
/page endpoint
GET /page/234 does return a 404 when the object is not found. Same comment about the response body being JSON, only do JSON when there is a purpose otherwise a string or empty. Another argument as to why /project/234 needs to be 404.
Note that this endpoint appears to only be stubbed out so it does not need much attention right now, nothing you do with it will return an AnnotationPage from a Project in the db. No other functionality tests were performed.
/line endpoint
Note that this endpoint is outside the scope of this PR
/my/projects endpoint
POST /my/projects gives a 404 where other endpoints 405. Not really a problem, but we should be consistent.
/my/profile endpoint
POST /my/projects gives a 404 where other endpoints 405. Not really a problem, but we should be consistent.
/user endpoint
GET /user/123 results in a 200 with a JSON response, like with /project. The best RESTful practice for this is to use a response code of 404. A response body is not strictly necessary, and we should only be returning JSON if it is necessary. The response body here would be much better as a string, or empty.
POST /user/123 gives a 404 where other endpoints 405. Not really a problem, but we should be consistent.
TESTS
Even with the issues mentioned above, all tests are passing. I am not sure what all is in scope, but it would be nice to have tests that verify the correct response code is used and the correct response body is returned. Otherwise, manual tests are strictly required as part of the review process even before a PR reaches this stage. We should decide what changes to implement and what is core enough for the code that it warrants real tests to rely on as a means to reduce what manual testing is required.
Below are manual tests performed by pulling down the PR, running the TPEN Services API, and using Talend API as a means to use the TPEN Services API from the perspective of an application. Note that only functionality interpreted as incorrect is reported, the code itself was not looked at. Essentially, this is a beta tester's report.
As a general note, I consider these things a barrier to PR approval and they make me unsure of exactly what to review and how to review it, and what things to 'just take care of' as I review it.
auth
/project endpoint
GET /project/123 results in a 200 with a JSON response. The json has a 'message' key that reports "No TPEN3 project with ID '123' found". The best RESTful practice for this is to use a response code of
404. A response body is not strictly necessary, and we should only be returning JSON if it is necessary. The response body here would be much better as a string, or empty.GET /project/ as a comparison to what is in the previous bullet point.
POST /project/create/ with a 'valid' Project object as the request body. Results in a 200 with a JSON response. My attempt to mess up
@typeandcreatorwere caught, but I successfully submitted the incorrect@context. That@contextvalue should work exactly like@type; it is a constant value that can't be overruled and is not required as part of the JSON body. It also allows me to save foreign keys into the database, such as{"hack": "db.tpen.drop()"}. The best RESTful practice for creating an object is to use a response code of201with aLocationheader that has id of the object created. A body is not strictly necessary, but handing back the JSON that is the created object is desired here and working correctly.DELETE /project/create which we expect to error out as a 405. It does, however the response is JSON with a message. We should not use JSON unless there is a purpose to. The response body would be better as a string, or empty.
POST project/import?createFrom=url with a valid body { "url":"https://iiif.io/api/cookbook/recipe/0001-mvm-image/manifest.json" }. It does not appear to be functional.
PATCH project/import?createFrom=url which we expect to error out as a 405. It does, however the response is JSON with a message. We should not use JSON unless there is a purpose to. The response body would be better as a string, or empty.
/manifest endpoint
/page endpoint
/line endpoint
/my/projects endpoint
/my/profile endpoint
/user endpoint
GET /user/123 results in a 200 with a JSON response, like with /project. The best RESTful practice for this is to use a response code of
404. A response body is not strictly necessary, and we should only be returning JSON if it is necessary. The response body here would be much better as a string, or empty.POST /user/123 gives a 404 where other endpoints 405. Not really a problem, but we should be consistent.
TESTS
Even with the issues mentioned above, all tests are passing. I am not sure what all is in scope, but it would be nice to have tests that verify the correct response code is used and the correct response body is returned. Otherwise, manual tests are strictly required as part of the review process even before a PR reaches this stage. We should decide what changes to implement and what is core enough for the code that it warrants real tests to rely on as a means to reduce what manual testing is required.