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

Add a new endpoint GET /benefits/<identity id> to the user benefits API #2606

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

tjmw
Copy link
Member

@tjmw tjmw commented Dec 18, 2024

What does this change?

Add a new endpoint /benefits/<identity id> which is auth'd using an x-api-key header (enforced in API Gateway). Because the auth is performed differently, at the API Gateway, I've migrated to using the GuApiGatewayWithLambdaByPath with a lambda per endpoint.

This will be called from support-frontend to prevent a user who already has a the adFree benefit from purchasing Guardian Ad-Lite.

Note: in the first implementation the endpoint does not know anything about internal users

How to test

I've deployed to CODE and tested.

Note: this requires a dangerous deploy in Riff Raff because the old lambda is deleted.

How can we measure success?

Have we considered potential risks?

Images

Accessibility

tjmw added 8 commits December 18, 2024 16:42
We'll be adding a new path shortly and I think it makes sense to keep
this lambda separate as the auth is different.
We'll have a single index and zip file for this handler and the new one.
@tjmw tjmw force-pushed the tw/user-benefits-lambda-by-path branch from 3e4bd57 to 73c5301 Compare December 20, 2024 15:01
@tjmw tjmw changed the title User benefits API -> GuApiGatewayWithLambdaByPath Add a new endpoint GET /benefits/<identity id> to the user benefits API Dec 20, 2024
@@ -27,34 +27,20 @@ jest.mock('@modules/product-benefits/userBenefits', () => ({
getUserBenefits: () => ['adFree'],
}));

describe('handler', () => {
it('returns a 404 for an unrecognized path or HTTP method', async () => {
Copy link
Member Author

@tjmw tjmw Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This diff is a bit messy but basically all that's happened here is the removal of the old 404 test, this now happens upstream and not in the lambda.

@tjmw tjmw marked this pull request as ready for review December 20, 2024 15:24

const userBenefitsResponse = await getUserBenefitsResponse(
stage,
new ProductCatalogHelper(await productCatalog.get()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe create a Lazy<ProductCatalogHelper> at the top level rather than a productCatalog and then creating a new helper every time?

};
}
};
export { benefitsMeHandler } from './benefitsMe';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this file now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This acts as the single entry point for esbuild which gives access to both handlers (but means we can still keep the handlers in separate files).

Copy link
Member

@rupertbates rupertbates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants