feat(GlobalRecs): Add /v3/firefox/global-recs API #53
Conversation
bassrock
left a comment
bassrock
left a comment
There was a problem hiding this comment.
@mmiermans this looks right to me, but i'm gonna defer a 🟢 to someone a bit closer to this code. If I happen to be that person still ping me and i'll check the box.
jeshuaborges
left a comment
jeshuaborges
left a comment
There was a problem hiding this comment.
Looks great to me. I love the strategy of getting the endpoint set up with mocks as a first PR.
| return next(variables); | ||
| } | ||
|
|
||
| const graphRes = await Recommendations({ |
There was a problem hiding this comment.
For future enhancement it: Given all requests in this repo is probably forwarding a request to the graph it would be great to have either a middleware or some factory to generate a graph client for us with all of the authentication parameters already handled.
|
|
||
| import { APIError, APIErrorResponse, BFFFxError } from '../../bfffxError'; | ||
|
|
||
| describe('input.ts recommendations query parameters', () => { |
There was a problem hiding this comment.
Great example of tests and covering all the conditionals in your unit tests.
src/api/v3/response.spec.ts
Outdated
| describe('response', () => { | ||
| it('ajv returns errors when bad objects', () => { | ||
| const valid = validate({}); | ||
| expect(!valid).toBeTruthy(); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
I copy-pasted these tests from the v1/desktop/recommendations endpoint. 😏
| }); | ||
|
|
||
| describe('responseTransformer', () => { | ||
| // all fields are non-nullable, not much to test other than happy path |
There was a problem hiding this comment.
There are other tests but you are handling those in your other unit tests.
| startTime: | ||
| type: integer | ||
| endTime: | ||
| type: integer | ||
| weightPosition: | ||
| type: number |
There was a problem hiding this comment.
I have little context around this but if we mean to represent these types as Typescript types then we'll have to use number for all of them 🤔
Unless this is just for the openapi schema?
There was a problem hiding this comment.
Yeah, this is an openapi schema, which distinguishes between integers and floating-point numbers.
| expect(errors).toEqual( | ||
| expect.objectContaining<APIErrorResponse>({ | ||
| errors: expect.arrayContaining<Array<APIError>>([ | ||
| expect.objectContaining<APIError>({ | ||
| status: '400', | ||
| title: 'Bad Request', | ||
| }), | ||
| ]), | ||
| }) | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Nice! Wondering if we can use toMatchObject like above 🤔
There was a problem hiding this comment.
I copy-pasted these, so I'm not 100% sure, but I believe that APIError contains additional attributes that we didn't want to lock in tests? I don't know which one is better in this case.
| excerpt: recommendation.corpusItem.excerpt, | ||
| domain: recommendation.corpusItem.publisher, | ||
| raw_image_src: recommendation.corpusItem.imageUrl, | ||
| image_src: `https://img-getpocket.cdn.mozilla.net/direct?url=${encodedImageUrl}&resize=w450`, |
There was a problem hiding this comment.
nit: if we don't expect this url to change we can throw it somewhere in a config file too but it's fine this way too.
| ], | ||
| recsExpireTime: 5400, | ||
| spocsPerNewTabs: 0.5, | ||
| version: '6f605b0212069b4b8d3d040faf55742061a25c16', |
There was a problem hiding this comment.
Does this also always remain the same?
There was a problem hiding this comment.
Yeah, I should probably add a comment..
Goal
Add a backward-compatible NewTab recommendation endpoint /v3/firefox/global-recs.
I'd love feedback/perspectives on:
References
JIRA ticket:
Documentation: