feat(browseRequests): add browse endpoints with test and documentation in progress #297

akhilesh26 · 2019-08-04T15:55:05Z

Problem

Implementation of Browse endpoints.

Solution

Start writing tests for those endpoints

Areas of Impact

coveralls · 2019-08-04T15:59:46Z

Coverage increased (+0.9%) to 61.422% when pulling f0808f4 on akhilesh26:phase2_api into 9b92daa on bookbrainz:master.

akhilesh26 · 2019-08-04T16:03:07Z

Hi @MonkeyDo,
Here the initial phase of tests for browse endpoints. My idea is to return the list of entity bbid instead of entity data. First, User will browse the entity bbid related to another entity, Then fetch the detail of an entity with BBID by using lookup requests if required. What is your plan to browse endpoints?
Suggest more endpoints for browse requests with the requirement explanation and check these test written in this PR. Thanks!

MonkeyDo

Good to see you start with the tests!

Looking at your proposal document, there are linked entities in your tests that weren't originally planned. For example, /author?publisher=….
I think for now you should limit to the planned links, and maybe think about adding other ones after the GSOC project.
For reference, here is what was originally planned:

<ENTITY>                  <LINKED_ENTITY>
/work                     edition, author
/edition                  work, author, edition-group, publisher
/edition-group            edition, author
/author                   work, edition
/publisher                edition, work

test/src/api/routes/test-edition-group.js

akhilesh26 · 2019-08-12T19:08:55Z

Hi @MonkeyDo
I refactored test cases according to option-4, Please look it once for more improvements, When you have time. Thanks!

test/src/api/helpers.js

src/api/helpers/middleware.js

src/api/helpers/utils.js

akhilesh26 · 2019-08-18T06:21:25Z

Hi @MonkeyDo,
Browse request is working for /author?work=<bbid> and /author?edition=<bbid>. Please help to get the required format response and validate the URL for the author's browser request as commented above. I will implement all browse endpoint on the basis of this.

src/api/routes/edition.js

akhilesh26 · 2019-08-22T03:34:39Z

@MonkeyDo Hi,
I updated filter validation according to the proposal, I think, all functionalities working now for browse request except edition & edition-group case. Please go through once and let me know if anything more on functionality. Then I will update documentation and tests also. Thanks!

MonkeyDo

I'm glad to see you making good progress!
We're just around the corner from the deadline, and at that pace I think you'll make it!

Let me know if you have any questions regarding the feedback below.

MonkeyDo · 2019-08-22T10:59:11Z

src/api/routes.js

 *      sortName:
 *        type: string
- *        example: 'Heinlein, Robert A.'
+ *        example: '<Sort name of enity>'


enity -> entity

src/api/routes/author.js

src/api/helpers/utils.js

MonkeyDo · 2019-08-22T11:25:28Z

src/api/routes/author.js

+		);
+		return res.status(200).send({
+			bbid: req.query.bbid,
+			relatedAuthors: authorRelationshipList


Considering we are choosing to allow only one type of related entity at a time, would it be simpler for users to only have a related or relatedEntities property for all browse endpoints results, instead of a different one for each endpoint (relatedAuthors, relatedWorks, …) ?
What do you think?

it gives the clarity on response.

I see your point.
I would then suggest rolling back to what you originally had in the tests and proposal, and removing the related part to have the response structure be {bbid:…, authors:[…]}.

src/api/routes/edition.js

src/api/routes/publisher.js

src/api/routes/work.js

src/server/routes/search.js

test/src/api/helpers.js

akhilesh26 · 2019-08-26T14:22:55Z

@MonkeyDo Hi!
Thanks for the detailed review, I will try to resolve all these ASAP. The feeling was not well in the because of the headache in last days, so not able to complete these, but I will try to complete all ASAP. Thanks for such support.
I added all most all the tests for work browse request, I request you to look at those and if they are implemented correctly then I will extend for all entities.

MonkeyDo

The test{Entity}BrowseRequest helpers don't currently allow you to test if the filters are working or not.
I would like to see less tests (I don't think there is any use in testing each filter-entity combination), but three tests specifically designed to ensure that each filter works, and that they work combined.
This will require a bit of rewrite. You'll need to set up the necessary works of different types and languages, and ensure that for each filter and for combined filters:
• you get 2 results if calling without the filter, and only one result with the filter
• without filter, there are different types/languages, and only one type if with the filter

That way we'll be sure that we don't break the filters mechanism in the future, rather that just testing that the query parameters are accepted.

This will ned to be done for each entity, as the filtering function is different for each.

endurance21 · 2020-02-22T06:49:21Z

@MonkeyDo
I see this work, unfinished
From where should I get started on this?..

MonkeyDo · 2020-02-22T20:35:59Z

Hi @endurance21!
It would be quite useful if you wanted to pick this API project back up! I believe most of the coding part for this PR is completed, and only the tests require completion.

The API has lookup endpoints and browse endpoints.
The lookup endpoints you can test live (with its live documentation along with it) at https://api.test.bookbrainz.org/1/api-docs/
You can find a description of the entire project here (a GSOC project from last year): https://community.metabrainz.org/t/gsoc-2019-json-web-api-for-bookbrainz/421988

I think the best way to get started on it would be to do a PR review yourself, meaning reading through this PR code and pointing out anything that looks incorrect or could be improved upon
.
Then the next obvious step would be to fix the requested changes from my last review.

After that, we can take it from there and I will do another full review with a view to getting this PR merged :)

Don't hesitate if you have questions!

endurance21 · 2020-02-23T07:24:43Z

@MonkeyDo
Okay.

MonkeyDo · 2020-02-23T08:08:59Z

I forgot some useful details you can find here: https://github.com/bookbrainz/bookbrainz-site#running-the-api

prabalsingh24 · 2020-04-24T13:47:40Z

This is the response for http://localhost:9098/1/work?author=4e8f8704-7c34-4a00-8ce5-f02bf26ca7df

Do you think this should be having alias of the author?

MonkeyDo · 2020-04-24T13:49:50Z

This is the response for http://localhost:9098/1/work?author=4e8f8704-7c34-4a00-8ce5-f02bf26ca7df

Do you think this should be having alias of the author?

No, we want people to fetch that —possibly beforehand— with /author/4e8f8704-7c34-4a00-8ce5-f02bf26ca7df

Before author browse endpoint tests

prabalsingh24 · 2020-04-27T20:10:19Z

@MonkeyDo Have a look at this when you're free :)

MonkeyDo

This is going really well, nice work !
It's really nice to see you're now quite comfortable with the testing framework, and thinking about what tests to write.

The route docs (inline swagger docs) still need a bit of love to match what's actually accepted for each browse endpoint (the accepted parameters and summaries don't match what's in the code).

On a more general note, I'm having serious second thought's about returning 406 errors when passed an invalid BBID. I think I was the one to decide that way back when last year, and it was probably a mistake.

406 Not Acceptable
This response is sent when the web server, after performing server-driven content negotiation, doesn't find any content that conforms to the criteria given by the user agent.

That doesn't seem adapted at all :/
I think instead a 400 should be returned. Along with the existing error message ('BBID is not valid uuid') I think that's clear enough.
The error code will need changing in the makeEntityLoader helper, in the route documentation and route tests.

src/api/helpers/middleware.js

src/api/routes/edition-group.js

src/api/routes/edition.js

src/api/routes/publisher.js

test/src/api/routes/test-edition-group.js

test/src/api/routes/test-publisher.js

test/src/api/routes/test-work.js

prabalsingh24 · 2020-04-29T06:22:34Z

I think instead a 400 should be returned. Along with the existing error message ('BBID is not valid uuid') I think that's clear enough.

Aaah, This makes sense now. I was wondering why we did 400 error for this invalid-bbid-pr and 406 here.

reduce the number of entities created while testing(5->2)

MonkeyDo

Looking good !

src/api/routes/author.js

src/api/routes/edition-group.js

src/api/routes/author.js

src/api/routes/edition-group.js

src/api/routes/edition.js

src/api/routes/publisher.js

test/src/api/routes/test-author.js

Refactored to avoid fetching the entity twice. In the middleware to load the entity, ensure we're loading extra related elements that are not simple relationships

Both in Lookup request results and browse request filter parameters

And clean up language_set table in truncate helper

MonkeyDo · 2020-06-19T11:21:37Z

🎉

MonkeyDo requested changes Aug 6, 2019

View reviewed changes

test/src/api/routes/test-edition-group.js Outdated Show resolved Hide resolved

MonkeyDo mentioned this pull request Aug 9, 2019

Deployment Docker setup for test API #298

Merged

akhilesh26 force-pushed the phase2_api branch from c387c18 to 279f12a Compare August 12, 2019 19:01

MonkeyDo reviewed Aug 14, 2019

View reviewed changes

test/src/api/helpers.js Outdated Show resolved Hide resolved

akhilesh26 commented Aug 18, 2019

View reviewed changes

src/api/helpers/middleware.js Outdated Show resolved Hide resolved

akhilesh26 commented Aug 18, 2019

View reviewed changes

src/api/helpers/utils.js Outdated Show resolved Hide resolved

akhilesh26 changed the title ~~feat(tests): add tests for browse endpoints~~ feat(tests): add browse endpoints with test and documentation in progress Aug 18, 2019

akhilesh26 changed the title ~~feat(tests): add browse endpoints with test and documentation in progress~~ feat(browseRequests): add browse endpoints with test and documentation in progress Aug 18, 2019

akhilesh26 commented Aug 22, 2019

View reviewed changes

src/api/routes/edition.js Outdated Show resolved Hide resolved

MonkeyDo requested changes Aug 22, 2019

View reviewed changes

MonkeyDo requested changes Aug 26, 2019

View reviewed changes

prabalsingh24 force-pushed the phase2_api branch 3 times, most recently from 576fb17 to 41b3ee1 Compare April 24, 2020 13:18

Akhilesh Kumar and others added 4 commits April 24, 2020 20:13

feat(tests): add tests for browse endpoints

4947de4

refactor(tests): modify test according to new response structure

27dcc96

fix: browse edition group for a publisher

4413ff0

test: Add relationships to author browse tests

c7c4947

Before author browse endpoint tests

tests : add tests for browse publisher

5ad5e19

prabalsingh24 requested a review from MonkeyDo April 27, 2020 20:09

MonkeyDo requested changes Apr 28, 2020

View reviewed changes

prabalsingh24 added 3 commits May 6, 2020 13:43

relationship: fix publisher-edition relationship

b91bbe6

tests: add test for edition-publisher relationship

4093dbe

fix: change 406 error to 400 error for invalid bbid

6ee4f0f

reduce the number of entities created while testing(5->2)

prabalsingh24 force-pushed the phase2_api branch from b7e3522 to 6ee4f0f Compare May 6, 2020 17:58

prabalsingh24 requested a review from MonkeyDo May 6, 2020 18:10

test: add tests for no related entity

a66fd02

prabalsingh24 force-pushed the phase2_api branch from efc1555 to a66fd02 Compare May 7, 2020 06:45

docs: make changes in the swagger docs

dc25372

prabalsingh24 force-pushed the phase2_api branch from 1e7fda5 to dc25372 Compare May 7, 2020 08:54

MonkeyDo requested changes May 7, 2020

View reviewed changes

MonkeyDo added 10 commits June 17, 2020 19:39

docs: polish up swagger docs

16ce3c4

docs: Fix swagger docs

f97c1b8

refactor(api): don't load Edition twice in EG brose request

918c95d

refactor(api): Require entity in loadEntity util

b5a70ef

refactor(api browse): Load browsed entities with related items

0ff10d8

Refactored to avoid fetching the entity twice. In the middleware to load the entity, ensure we're loading extra related elements that are not simple relationships

refactor(api): Use ISO codes for languages

6102b11

Both in Lookup request results and browse request filter parameters

test(api): Test route error when browsing by multiple entities

56b5666

refactor(api): Use ISO codes from alias languages

dec997c

Merge branch 'master' into pr/297

e791a62

test(api): Fix author creation helper

6f90f7a

MonkeyDo force-pushed the phase2_api branch from 7d839cd to 6f90f7a Compare June 19, 2020 10:28

MonkeyDo added 2 commits June 19, 2020 12:45

refactor(api): Don't import all of lodash

5db80e2

refactor(tests): Small refactor for test work creation

f0808f4

And clean up language_set table in truncate helper

MonkeyDo merged commit 8cfe173 into metabrainz:master Jun 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(browseRequests): add browse endpoints with test and documentation in progress #297

feat(browseRequests): add browse endpoints with test and documentation in progress #297

akhilesh26 commented Aug 4, 2019

coveralls commented Aug 4, 2019 •

edited

Loading

akhilesh26 commented Aug 4, 2019

MonkeyDo left a comment

akhilesh26 commented Aug 12, 2019

akhilesh26 commented Aug 18, 2019

akhilesh26 commented Aug 22, 2019

MonkeyDo left a comment

MonkeyDo Aug 22, 2019

MonkeyDo Aug 22, 2019

akhilesh26 Aug 26, 2019

MonkeyDo Aug 26, 2019

akhilesh26 commented Aug 26, 2019

MonkeyDo left a comment

endurance21 commented Feb 22, 2020

MonkeyDo commented Feb 22, 2020

endurance21 commented Feb 23, 2020

MonkeyDo commented Feb 23, 2020

prabalsingh24 commented Apr 24, 2020

MonkeyDo commented Apr 24, 2020

prabalsingh24 commented Apr 27, 2020

MonkeyDo left a comment

prabalsingh24 commented Apr 29, 2020

MonkeyDo left a comment

MonkeyDo commented Jun 19, 2020

feat(browseRequests): add browse endpoints with test and documentation in progress #297

feat(browseRequests): add browse endpoints with test and documentation in progress #297

Conversation

akhilesh26 commented Aug 4, 2019

Problem

Solution

Areas of Impact

coveralls commented Aug 4, 2019 • edited Loading

akhilesh26 commented Aug 4, 2019

MonkeyDo left a comment

Choose a reason for hiding this comment

akhilesh26 commented Aug 12, 2019

akhilesh26 commented Aug 18, 2019

akhilesh26 commented Aug 22, 2019

MonkeyDo left a comment

Choose a reason for hiding this comment

MonkeyDo Aug 22, 2019

Choose a reason for hiding this comment

MonkeyDo Aug 22, 2019

Choose a reason for hiding this comment

akhilesh26 Aug 26, 2019

Choose a reason for hiding this comment

MonkeyDo Aug 26, 2019

Choose a reason for hiding this comment

akhilesh26 commented Aug 26, 2019

MonkeyDo left a comment

Choose a reason for hiding this comment

endurance21 commented Feb 22, 2020

MonkeyDo commented Feb 22, 2020

endurance21 commented Feb 23, 2020

MonkeyDo commented Feb 23, 2020

prabalsingh24 commented Apr 24, 2020

MonkeyDo commented Apr 24, 2020

prabalsingh24 commented Apr 27, 2020

MonkeyDo left a comment

Choose a reason for hiding this comment

prabalsingh24 commented Apr 29, 2020

MonkeyDo left a comment

Choose a reason for hiding this comment

MonkeyDo commented Jun 19, 2020

coveralls commented Aug 4, 2019 •

edited

Loading