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.

Sign up for GitHub

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

Jump to bottom

Revamp JoinRecords #93

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

Revamp JoinRecords #93

wants to merge 39 commits into from

Conversation

WillNilges
Copy link
Collaborator

@WillNilges WillNilges commented Oct 21, 2024 •
edited
Loading

This PR does a number of things:

  • Adds HTTP response code, submission time, number of replays, and install number to JoinRecords. This makes it easier to track Join Requests and
  • Introduces the concept of Join Record, which is simply a Join Request plus the stuff mentioned above
  • Improves(?) the JoinRecord code
  • Adds a test to ensure JoinRecords are saved as expected, and updated with the HTTP code and Install Number.

@WillNilges WillNilges changed the base branch from wdn/202 to main October 28, 2024 12:05
@WillNilges WillNilges changed the base branch from main to wdn/202 October 28, 2024 12:06
@WillNilges WillNilges changed the base branch from wdn/202 to main October 28, 2024 12:06
@WillNilges WillNilges changed the title Stub out updating S3 submissions with httpCode Add HTTP Code to JoinRecords Oct 29, 2024
@WillNilges WillNilges mentioned this pull request Oct 31, 2024
Open
@WillNilges WillNilges changed the title Add HTTP Code to JoinRecords Revamp JoinRecords Nov 1, 2024
WillNilges
WillNilges commented Nov 1, 2024
View reviewed changes
app/join_record.ts Outdated Show resolved Hide resolved
@WillNilges WillNilges marked this pull request as ready for review November 1, 2024 13:27
Andrew-Dickinson
Andrew-Dickinson reviewed Nov 2, 2024
View reviewed changes
.github/workflows/playwright.yml Outdated Show resolved Hide resolved
app/join_record.ts Outdated Show resolved Hide resolved
app/join_record.ts Outdated Show resolved Hide resolved
app/join_record.ts
Comment on lines +79 to +80
// Oof, guess we'll drop this on the floor.
console.error(err);
Copy link
Member

Choose a reason for hiding this comment

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

Can we set up some kind of datadog alert for this at least?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can definitely emit a metric. TBH I was trying to avoid setting up metrics for as long as possible 😂

components/JoinForm/JoinForm.tsx Outdated Show resolved Hide resolved
components/JoinForm/JoinForm.tsx Outdated Show resolved Hide resolved
@WillNilges WillNilges force-pushed the wdn/updated-s3-submissions branch 8 times, most recently from 3ad7fa9 to 3dcc100 Compare November 4, 2024 05:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Andrew-Dickinson Andrew-Dickinson Andrew-Dickinson left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@WillNilges @Andrew-Dickinson