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

Contribution cron job #70

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

Conversation

TevinBeckwith
Copy link
Contributor

@TevinBeckwith TevinBeckwith commented Nov 10, 2021

Pre-Requisites

  • Yes, I updated Authors.md OR this is not my first contribution
  • Yes, I included and/or modified tests to cover relevant code OR my change is non-technical
  • Yes, I wrote this code entirely myself OR I properly attributed these changes in Third Party Notices

Description of Changes

This PR allows for the continual fetching of user contribution data and saving it to our database

Related Issues

@TevinBeckwith TevinBeckwith marked this pull request as draft November 10, 2021 07:09
@github-actions github-actions bot added the migration may be needed An entity file was modified, a migration may be needed label Nov 10, 2021
NotKevin pushed a commit to NotKevin/Project-X that referenced this pull request Nov 23, 2021
Copy link
Contributor

@SpencerKaiser SpencerKaiser left a comment

Choose a reason for hiding this comment

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

This is a great start, but I don't think we did a good job of communicating the expectations for structure... I added some comments about using a library to help with the cron job stuff and the only other big change is that I think we should move the entirety of that endpoint out into utility functions and not expose them to the API. By that I mean we should probably have api/src/utils/github files for each of the major commands, each with just the interfaces that relate to what they do, and then we should have a function within a new root level file (maybe something like src/cronJobs.ts) that exports a single startCronJobs method you call from index.ts.

Let me know if you have any questions on the above; we can always hop on a call to chat. Beyond that, please don't write any more tests until we do another review of the cron stuff, just because I'm worried we'll need a few more rounds of revisions given the complexity of all this 😬

packages/api/src/api/contributions.ts Outdated Show resolved Hide resolved
packages/api/src/api/contributions.ts Outdated Show resolved Hide resolved
packages/api/src/api/contributions.ts Outdated Show resolved Hide resolved
packages/api/src/api/contributions.ts Outdated Show resolved Hide resolved
packages/api/src/api/contributions.ts Outdated Show resolved Hide resolved
packages/api/src/api/contributions.ts Outdated Show resolved Hide resolved
packages/api/src/api/contributions.ts Outdated Show resolved Hide resolved
packages/api/src/entities/User.ts Show resolved Hide resolved
packages/api/src/index.ts Outdated Show resolved Hide resolved
packages/api/src/index.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@dre8597 dre8597 left a comment

Choose a reason for hiding this comment

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

Overall looks good to me, with just some small nit-picky changes for better clarity.

packages/api/src/utils/github/buildContributionQuery.ts Outdated Show resolved Hide resolved
packages/api/src/utils/github/buildProjectsQuery.ts Outdated Show resolved Hide resolved
packages/api/src/utils/github/buildProjectsQuery.ts Outdated Show resolved Hide resolved
packages/api/src/utils/github/buildProjectsQuery.ts Outdated Show resolved Hide resolved
packages/api/src/utils/github/buildProjectsQuery.ts Outdated Show resolved Hide resolved
packages/api/src/utils/github/buildUserQuery.ts Outdated Show resolved Hide resolved
packages/api/src/utils/github/buildProjectsQuery.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@dre8597 dre8597 left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Copy link
Contributor

@SpencerKaiser SpencerKaiser left a comment

Choose a reason for hiding this comment

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

Getting close! Awesome work; just a few more tweaks and then we can work on testing.

README.md Outdated Show resolved Hide resolved
packages/api/package.json Outdated Show resolved Hide resolved
packages/api/src/cronJobs.ts Outdated Show resolved Hide resolved
packages/api/src/cronJobs.ts Outdated Show resolved Hide resolved
packages/api/src/cronJobs.ts Outdated Show resolved Hide resolved
packages/api/src/utils/github/buildProjectsQuery.ts Outdated Show resolved Hide resolved
packages/api/src/utils/github/buildProjectsQuery.ts Outdated Show resolved Hide resolved
packages/api/src/utils/github/buildUserQuery.ts Outdated Show resolved Hide resolved
packages/api/tsconfig.json Outdated Show resolved Hide resolved
@TevinBeckwith TevinBeckwith marked this pull request as ready for review December 3, 2021 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
migration may be needed An entity file was modified, a migration may be needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants