Skip to content

Conversation

@johnduffell
Copy link
Member

@johnduffell johnduffell commented Oct 8, 2025

TODO complete #3141 and merge into this branch before merging to main
Also see further enhancements in https://github.com/guardian/support-service-lambdas/tree/jd/discount-eligibility-list-subchecks which can be merged afterwards


At the moment in order to find out what discounts there are and the eligibility, we have to look at the source code.

This is tricky for devs, and impossible for non devs.

In the benefits api, a human readable endpoint was added, which is useful (that one is at https://user-benefits.guardianapis.com/benefits/list )

This PR, adds a similar endpoint to discount api
image

available on CODE if the branch is deployed https://discount-api-code.support.guardianapis.com/docs

},
],
},
"Type": "AWS::IAM::Role",
Copy link
Member Author

Choose a reason for hiding this comment

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

be sure to expand all diffs that are not loaded e.g. above
image

},
"Type": "AWS::ApiGateway::ApiKey",
},
"discountapilambdadiscountapiCODEdocsE0CCF3F7": {
Copy link
Member Author

@johnduffell johnduffell Oct 8, 2025

Choose a reason for hiding this comment

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

there is a new resource in the api gateway to cover /docs and a nested one for anything under that
image

ref https://eu-west-1.console.aws.amazon.com/apigateway/main/apis/qdsjxf9ie2/resources?api=qdsjxf9ie2&region=eu-west-1

},
"discountapilambdadiscountapiCODEdocsGET29AC0E0B": {
"Properties": {
"ApiKeyRequired": false,
Copy link
Member Author

Choose a reason for hiding this comment

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

image

},
],
},
"Type": "AWS::IAM::Role",
Copy link
Member Author

Choose a reason for hiding this comment

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

you can see the changeset is as expected:

  • the grouped sections are the /docs and /docs/* resources and their associated things
  • The top has the replacement Deployment,
  • the Stage is updated to point to the new deployment.
image

"UpdateReplacePolicy": "Retain",
},
"metricpushapilambdametricpushapiCODEDeployment9B65369C9b8e195f5ba5aaa1cc9e4b269b959bd9": {
"metricpushapilambdametricpushapiCODEDeployment9B65369Cbf500934fd3289703fbdfb0b9c7f32c9": {
Copy link
Member Author

@johnduffell johnduffell Oct 8, 2025

Choose a reason for hiding this comment

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

some minor regeneration of the Deployment due to the properties of the Rest API being changed.
This is intentional by CDK, (although it needn't happen as eagerly as it does)

restApiName: getNameWithStage(scope, props.nameSuffix),
description: props.apiDescriptionOverride ?? 'API Gateway created by CDK',
proxy: true,
proxy: false, // add proxy method later, to allow public paths
Copy link
Member Author

Choose a reason for hiding this comment

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

if you set this to true, any later attempts to add new Resources to the API gateway is blocked with an error. If you set to false and add the proxy method yourself, you can also add the extra Resources you need later.

};
}

const render = (title: string, headers: string[], rows: string[][]): string => {
Copy link
Member Author

Choose a reason for hiding this comment

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

adapted from

const getHtmlBody = (): string => {
return `
<!DOCTYPE html>
<html lang="en">
<head>
<title>Product Benefits List</title>
<meta name="robots" content="noindex">
<style>
table {
border-collapse: collapse;
}
th, td {
border: 1px solid black;
padding: 8px;
text-align: left;
}
th {
background-color: #CCC;
}
tr:nth-child(odd) {
background-color: #EEE;
}
</style>
</head>
<body>
<h1>Product Benefits List</h1>
<table>
<tbody>
<tr>
<th>Zuora Product</th>
<th>Customer Facing Name</th>
<th>Benefits</th>
${Object.entries(productBenefitMapping)
.map(
([key, value]) =>
`<tr>` +
`<td>${key}</td>` +
`<td>${getCustomerFacingName(key)}</td>` +
`<td>${value.join(', ')}</td>` +
`</tr>`,
)
.join('')}
</table>
</body>
</html>
`;

although we should probably make both use a common function at some point, so we can keep the styling in step

Comment on lines 28 to 31
const getEnv = (env: string): string =>
getIfDefined(process.env[env], `${env} environment variable not set`);

const stage = getEnv('STAGE') as Stage;
Copy link
Member Author

Choose a reason for hiding this comment

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

the old type was wrong, it could have been undefined, now it's prevented

Comment on lines 46 to 48
httpMethod: 'GET',
path: '/docs',
handler: () => docsHandler(stage),
Copy link
Member Author

Choose a reason for hiding this comment

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

perhaps we should specificcally mark the public endpoints here, and get the Router to assert that the api key was checked unless marked public. This could provide a layer of protection against endpoints accidentally being public by coding mistakes here.

// end fields that match the zuora catalog
emailIdentifier: DataExtensionName;
eligibilityCheckForRatePlan?: EligibilityCheck;
eligibilityCheckForRatePlan: EligibilityCheck;
Copy link
Member Author

Choose a reason for hiding this comment

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

not sure why it was optional, there's a specific NoCheck value.

import { stringify } from '../src/stringify';

test('stringify should not serialise any stray values in the object, for security reasons', () => {
const testSchema = z.object({
Copy link
Member Author

Choose a reason for hiding this comment

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

don't forget to open the Large Diff below...

Comment on lines +27 to +30
export type GenericProductCatalog = Record<
string,
z.infer<typeof baseProductSchema>
>;
Copy link
Member Author

Choose a reason for hiding this comment

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

when using the ProductCatalog type, it expects me to access it with catalog.Contribution.ratePlans.Monthly etc, however when trying to iterate it, it quickly resorts to any
By extracting a "Generic" type, this lets us keep the original types, while hopefully making sure it it also iterable in a sensible way.

Comment on lines +32 to +51
const baseRatePlanSchema = z.object({
id: z.string(),
charges: z.record(
z.string(),
z.object({
id: z.string(),
}),
),
pricing: z.record(z.string(), z.number()),
termLengthInMonths: z.number(),
termType: termTypeSchema,
});

const baseProductSchema = z.object({
active: z.boolean(),
billingSystem: z.enum(['zuora', 'stripe']),
customerFacingName: z.string(),
isDeliveryProduct: z.boolean(),
ratePlans: z.record(z.string(), baseRatePlanSchema),
});
Copy link
Member Author

Choose a reason for hiding this comment

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

I told copilot to extract the fields it could, although I'm happy to reduce it to only ratePlans and id if need be.

@johnduffell johnduffell force-pushed the jd/discount-eligibility-list branch from 2b58cc1 to d8dced2 Compare October 8, 2025 14:08
@johnduffell johnduffell marked this pull request as ready for review October 8, 2025 14:58
@johnduffell johnduffell requested a review from Copilot October 8, 2025 14:59
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds a human-readable HTML documentation endpoint to the discount API, allowing developers and non-developers to easily view available discounts and their eligibility requirements. This follows the pattern established in the benefits API.

  • Added /docs endpoint that renders an HTML table showing discount information
  • Refactored product catalog schema to reduce duplication with base schemas
  • Moved utility functions to separate modules for better organization

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
modules/product-catalog/src/productCatalogSchema.ts Refactored to use base schemas, reducing duplication and adding GenericProductCatalog type
handlers/discount-api/src/docsHandler.ts New handler that generates HTML documentation table for discounts
handlers/discount-api/src/index.ts Updated to add docs endpoint and moved stringify function
handlers/discount-api/src/stringify.ts Extracted stringify utility to separate module
handlers/discount-api/src/productToDiscountMapping.ts Made function exportable and eligibility check required
cdk/lib/discount-api.ts Added public path configuration for docs endpoint
cdk/lib/cdk/sr-api-lambda.ts Modified to support public paths without API key requirement

restApiName: getNameWithStage(scope, props.nameSuffix),
description: props.apiDescriptionOverride ?? 'API Gateway created by CDK',
proxy: true,
proxy: false, // add proxy method later, to allow public paths
Copy link

Copilot AI Oct 8, 2025

Choose a reason for hiding this comment

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

Setting proxy to false changes the default behavior for all API lambdas. This is a significant change that affects how all APIs handle routing and could break existing functionality that relies on proxy behavior.

Suggested change
proxy: false, // add proxy method later, to allow public paths
proxy: true, // use proxy integration for default routing behavior

Copilot uses AI. Check for mistakes.
Copy link
Member Author

@johnduffell johnduffell Oct 8, 2025

Choose a reason for hiding this comment

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

below I have added back the identical methods, which is shown in the CDK diff as not being a significant change

Comment on lines 28 to 31
const getEnv = (env: string): string =>
getIfDefined(process.env[env], `${env} environment variable not set`);

const stage = getEnv('STAGE') as Stage;
Copy link

Copilot AI Oct 8, 2025

Choose a reason for hiding this comment

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

Environment variables should be loaded using the appConfig pattern per coding guidelines. Use modules/aws/src/appConfig.ts to load config from SSM instead of directly accessing process.env.

Copilot generated this review using guidance from repository custom instructions.
Copy link
Member Author

Choose a reason for hiding this comment

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

it's a good point, actually STACK/STAGE/APP are not configuration in that sense, perhaps this needs clarifying in the custom instructions

# Conflicts:
#	cdk/lib/cdk/sr-api-lambda.ts
# Conflicts:
#	modules/product-catalog/src/productCatalogSchema.ts
# Conflicts:
#	handlers/discount-api/src/index.ts
@johnduffell johnduffell changed the base branch from main to jd/extract-restclient-rebase November 19, 2025 11:13
@johnduffell johnduffell force-pushed the jd/extract-restclient-rebase branch from 1a23ebc to 6061b8d Compare November 19, 2025 14:00
@johnduffell johnduffell force-pushed the jd/extract-restclient-rebase branch 2 times, most recently from dc5cf38 to c1b54e2 Compare November 19, 2025 21:03
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