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

Pong2 #9775

Merged
merged 6 commits into from
Oct 25, 2023
Merged

Pong2 #9775

merged 6 commits into from
Oct 25, 2023

Conversation

fiji-flo
Copy link
Contributor

@fiji-flo fiji-flo commented Oct 9, 2023

Summary

Remove unused Carbon fallback and support BSA.


How did you test this change?

Staging.

this refactors the pong lib:
- bundle click.js, get.js, viewed.js in pong.js
- add pong2.js based on bsa's api
- remove fallback
};

export default function anonymousIpByCC(countryCode) {
return cc2ip[countryCode] ?? "127.0.0.1";
return cc2ip[countryCode] ?? "10.10.10.10";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

IPs and fallback was provided by BSA.

@@ -37,8 +37,8 @@
proxyApi
);
router.all("/submit/mdn-yari/*", requireOrigin(Origin.main), proxyTelemetry);
router.all("/pong/*", requireOrigin(Origin.main), express.json(), proxyKevel);
router.all("/pimg/*", requireOrigin(Origin.main), proxyKevel);
router.all("/pong/*", requireOrigin(Origin.main), express.json(), proxyPong);

Check failure

Code scanning / CodeQL

Missing rate limiting High

This route handler performs
authorization
, but is not rate-limited.
This route handler performs
authorization
, but is not rate-limited.
cloud-function/src/app.ts Dismissed Show dismissed Hide dismissed
Copy link
Member

@LeoMcA LeoMcA left a comment

Choose a reason for hiding this comment

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

One thing actually seems like a bug (the copy -> description suggestion), the rest is relatively nitty - looks good!

cloud-function/README.md Show resolved Hide resolved
cloud-function/src/env.ts Outdated Show resolved Hide resolved
libs/pong/pong2.js Outdated Show resolved Hide resolved
},
};
});
const decisionRes = await Promise.all(requests);
Copy link
Member

Choose a reason for hiding this comment

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

Do we want all placements to fail if just one does, might Promise.allSettled be more appropriate here?

libs/pong/pong2.js Show resolved Hide resolved
libs/pong/pong2.js Outdated Show resolved Hide resolved
libs/pong/pong2.js Outdated Show resolved Hide resolved
Copy link
Contributor Author

@fiji-flo fiji-flo left a comment

Choose a reason for hiding this comment

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

I added a version in the payload to route as the click will not have the referrer set. We'll remove this after the migration.

libs/pong/pong2.js Outdated Show resolved Hide resolved
@fiji-flo fiji-flo requested a review from LeoMcA October 12, 2023 14:36
Copy link
Member

@LeoMcA LeoMcA left a comment

Choose a reason for hiding this comment

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

code looks good, attempted to test on stage but I don't think these latest changes have been deployed there?

@fiji-flo fiji-flo merged commit 4d9413d into main Oct 25, 2023
11 checks passed
@fiji-flo fiji-flo deleted the pong2 branch October 25, 2023 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants