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

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

Open
wants to merge 28 commits into
base: master
from

Conversation

@akhilesh26
Copy link
Contributor

commented Aug 4, 2019

Problem

Implementation of Browse endpoints.

Solution

Start writing tests for those endpoints

Areas of Impact

@coveralls

This comment has been minimized.

Copy link

commented Aug 4, 2019

Coverage Status

Coverage increased (+0.4%) to 43.516% when pulling 9e8cdc0 on akhilesh26:phase2_api into bdf0bb4 on bookbrainz:master.

@akhilesh26

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2019

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
Copy link
Contributor

left a comment

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
});

it('should return list of EditionGroup, Which is published by an Publisher', async function () {
const res = await chai.request(app).get(`/edition?edition-group=${aBBID}`);

This comment has been minimized.

Copy link
@MonkeyDo

MonkeyDo Aug 6, 2019

Contributor

Looks like you're browsing editions here, not edition groups.

The description of the test mentions publishers, so I suppose it's /edition-group?publisher=${aBBID}

@akhilesh26 akhilesh26 force-pushed the akhilesh26:phase2_api branch from c387c18 to 279f12a Aug 12, 2019

@akhilesh26

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

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

expect(res.body).to.be.an('object');
expect(res.body).to.have.all.keys(
'bbid',
'edtitionGroups'

This comment has been minimized.

Copy link
@MonkeyDo

MonkeyDo Aug 14, 2019

Contributor

editionGroups, not edtitionGroups

This comment has been minimized.

Copy link
@MonkeyDo

MonkeyDo Aug 14, 2019

Contributor

Same applies to lines 100 -> 110

This comment has been minimized.

Copy link
@MonkeyDo

MonkeyDo Aug 22, 2019

Contributor

These comments still apply



/* eslint-disable*/

export function validateAuthorBrowseRequest (req, res, next) {

This comment has been minimized.

Copy link
@akhilesh26

akhilesh26 Aug 18, 2019

Author Contributor

Please help to validate browse request here and This function return URL not valid if the request has invalid query params.

// Allow for capitalization mistakes? (.toLowercase() on both?)
if (relationship.targetEntityType === browsedEntityType) {
return {
entity: relationship.target,

This comment has been minimized.

Copy link
@akhilesh26

akhilesh26 Aug 18, 2019

Author Contributor

How to load entity basic detail here? relationship.target and relationship.source have only bbid and entity type.

@akhilesh26

This comment has been minimized.

Copy link
Contributor Author

commented Aug 18, 2019

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.

@akhilesh26 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 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

MonkeyDo and others added 3 commits Aug 20, 2019
feat: filter author browse requests by type
This is an example of how to filter browse request entities by a specific attribute
if (req.query.type) {
return _.toLower(_.get(relatedEntity, 'editionType.label')) === req.query.type;
const edtionFormatMatched = _.toLower(relatedEntity.editionFormat) === req.query.format;
const editionLanguageMatched = relatedEntity.includes(_.upperFirst(req.query.language));

This comment has been minimized.

Copy link
@akhilesh26

akhilesh26 Aug 22, 2019

Author Contributor

Here is used the format filter, instead of status. the format is more meaningful than status. Edition formats are paperback, hardcover, ebook extra.

This comment has been minimized.

Copy link
@MonkeyDo

MonkeyDo Aug 22, 2019

Contributor

I agree. You could also add a status filter, but I don't think it will be very used for now.

@akhilesh26

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

@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
Copy link
Contributor

left a comment

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.

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

This comment has been minimized.

Copy link
@MonkeyDo

MonkeyDo Aug 22, 2019

Contributor

enity -> entity

*/

router.get('/',
validateBrowseRequestQueryParameters(['edition', 'author', 'edition-group', 'work']),

This comment has been minimized.

Copy link
@MonkeyDo

MonkeyDo Aug 22, 2019

Contributor

'publisher' is missing here.
Please refer to this exhaustive list I sent you (the ones in parenthesis you can ignore):
https://www.irccloud.com/pastebin/raw/6wF8x1V7

if (entity.bbid === relationship.sourceBbid) {
// We need a good way to compare entity type strings here and same thing below
// Allow for capitalization mistakes? (.toLowercase() on both?)
if (relationship.target.type === browsedEntityType) {

This comment has been minimized.

Copy link
@MonkeyDo

MonkeyDo Aug 22, 2019

Contributor

Find a safer way to compare strings. At the moment, if my query parameter is 'Work' instead of 'work' it won't work. I think it would be easy and acceptable to allow for capitalization mistakes.
Once that is done, you can remove the comment above.

This comment has been minimized.

Copy link
@MonkeyDo

MonkeyDo Aug 22, 2019

Contributor

Same for line 77

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

This comment has been minimized.

Copy link
@MonkeyDo

MonkeyDo Aug 22, 2019

Contributor

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?

This comment has been minimized.

Copy link
@akhilesh26

akhilesh26 Aug 26, 2019

Author Contributor

it gives the clarity on response.

This comment has been minimized.

Copy link
@MonkeyDo

MonkeyDo Aug 26, 2019

Contributor

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:[…]}.

if (req.query.type) {
return _.toLower(_.get(relatedEntity, 'editionType.label')) === req.query.type;
const edtionFormatMatched = _.toLower(relatedEntity.editionFormat) === req.query.format;
const editionLanguageMatched = relatedEntity.includes(_.upperFirst(req.query.language));

This comment has been minimized.

Copy link
@MonkeyDo

MonkeyDo Aug 22, 2019

Contributor

I agree. You could also add a status filter, but I don't think it will be very used for now.


router.get('/',
validateBrowseRequestQueryParameters(['author', 'edition', 'edition-group', 'work', 'publisher']),
makeEntityLoader(null, utils.relationshipsRelations, 'Entity not found', true),

This comment has been minimized.

Copy link
@MonkeyDo

MonkeyDo Aug 22, 2019

Contributor

After some thinking and tinkering, I found a solution for the edition/edition-group here.

You can replace this makeEntityLoader line by an inline middleware where we will add the related editions to load, only if the queried type is Edition Group (otherwise bookshelf throws an error), then pass along to makeEntityLoader:

async (req, res, next) => {
		const relationships = utils.relationshipsRelations;
		if (req.query.modelType === 'EditionGroup') {
			// If we're loading an Edition Group, also load the related Editions from the ORM model
			relationships.push(...editionBasicRelations.map(rel => `editions.${rel}`));
		}
		await makeEntityLoader(null, relationships, 'Entity not found', true)(req, res, next);
	},

This means that further down in the /edition endpoint, you can access the related editions directly from the loaded entity:

if (req.query.modelType === 'EditionGroup') {
			const {editions} = res.locals.entity;
			const mappedEditions = editions
				.map(edition => ({
					entity: getEditionBasicInfo(edition)
				}))
				.filter(relationshipsFilterMethod);
			editionRelationshipList.push(...mappedEditions);
		}
loadEntityRelationshipsForBrowse(),
async (req, res, next) => {
function relationshipsFilterMethod(relatedEntity) {
const publisherTypeMacthed = _.toLower(relatedEntity.type) === req.query.type;

This comment has been minimized.

Copy link
@MonkeyDo

MonkeyDo Aug 22, 2019

Contributor

Typo: publisherTypeMacthed -> publisherTypeMatched

As described for the edition endpoint relationshipsFilterMethod, please reorganize the logic to ensure those matching assertions are only run when the relevant query param is passed.

loadEntityRelationshipsForBrowse(),
async (req, res, next) => {
function relationshipsFilterMethod(relatedEntity) {
const workTypeMatched = _.toLower(relatedEntity.workType) === req.query.type;

This comment has been minimized.

Copy link
@MonkeyDo

MonkeyDo Aug 22, 2019

Contributor

As described above for the edition and publisher endpoints relationshipsFilterMethod, please reorganize the logic

@@ -129,7 +129,7 @@ router.get('/reindex', auth.isAuthenticated, (req, res) => {
const {orm} = req.app.locals;
const indexPromise = new Promise((resolve) => {
// TODO: This is hacky, and we should replace it once we switch to SOLR.
const trustedUsers = ['Leftmost Cat', 'LordSputnik', 'Monkey', 'iliekcomputers'];
const trustedUsers = ['Leftmost Cat', 'LordSputnik', 'Monkey', 'iliekcomputers', 'mastcoder'];

This comment has been minimized.

Copy link
@MonkeyDo

MonkeyDo Aug 22, 2019

Contributor

As this will be deployed as is in production, please remove this.
It's definitely the right way to do it locally, but please don't commit that change :)

expect(res.body).to.be.an('object');
expect(res.body).to.have.all.keys(
'bbid',
'edtitionGroups'

This comment has been minimized.

Copy link
@MonkeyDo

MonkeyDo Aug 22, 2019

Contributor

These comments still apply

@akhilesh26

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

@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
Copy link
Contributor

left a comment

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.