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

Member archive reminder cronjob #613

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

Conversation

patriciaahuang
Copy link
Contributor

Summary

There will be a script made that archives members automatically each semester. This cronjob sends an email to the Ops Leads reminding them to upload the CSV from the returning members survey to archive members at the beginning of each semester.

Notion/Figma Link

https://www.notion.so/Member-archive-script-0540cb5e7d0349c4921f344b1875387e?pvs=4

Test Plan

Ran yarn ts-node --transpile-only scripts/send-member-archive-reminder.ts locally.

Breaking Changes

Need to add an ops-leads collection with emails as the documents in the prod DB.

@patriciaahuang patriciaahuang requested a review from a team as a code owner May 4, 2024 01:13
@dti-github-bot
Copy link
Member

dti-github-bot commented May 4, 2024

[diff-counting] Significant lines: 68.

Copy link
Contributor

@cchrischen cchrischen left a comment

Choose a reason for hiding this comment

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

This looks great, Patricia! The new cronjob follows the same style as the existing ones, and I like how you used the if checks to incorporate the new schedule with the preexisting ones.

I found that in checks, the one labeled ".github/..." fails. Github actions complains about "invalid workflow file", but I think it should be an easy fix.

Comment on lines 64 to 65
member-archive-reminder:
if: contains(github.event.schedule, '0 14 20 1,8 *')
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these two lines should be indented one more, so that member-archive-reminder is in jobs, and if is on the same indentation with runs-on like the other two jobs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think is a symptom of not having formatting on the YAML files. We shouldn't have to point these styling issues out :(

if: contains(github.event.schedule, '0 14 20 1,8 *')
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
Copy link
Contributor

Choose a reason for hiding this comment

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

In Oscar's PR #607, he updated the workflows, so this should be changed to actions/checkout@v4, along with lines 71 and 75. :)

Copy link
Contributor

@alyssayzhang alyssayzhang left a comment

Choose a reason for hiding this comment

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

Overall, great work on this PR 👍 !! There is a message You have an error in your yaml syntax on line 66 in the github workflows.

Copy link
Contributor

@kevinmram kevinmram left a comment

Choose a reason for hiding this comment

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

you've done an awesome job on this- it's lookin' good! However, I noticed in the cron configuration for 'profile-update-notifications' and 'tec-request-notification,' both jobs are triggered by the same cron schedule (0 23 * * MON), which may cause potential performance bottlenecks. 🐐

@henry-li-06
Copy link
Collaborator

you've done an awesome job on this- it's lookin' good! However, I noticed in the cron configuration for 'profile-update-notifications' and 'tec-request-notification,' both jobs are triggered by the same cron schedule (0 23 * * MON), which may cause potential performance bottlenecks. 🐐

Fyi this shouldn't be an issue unless we're saying that google/firebase and/or microsoft/github infra can't handle the load 😆

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.

7 participants