New routes for event-instance, attendance, and event-tags#142
New routes for event-instance, attendance, and event-tags#142evanpetzoldt merged 1 commit intodevfrom
event-instance, attendance, and event-tags#142Conversation
evanpetzoldt
requested review from
dnishiyama,
pstaylor-patrick and
taterhead247
as code owners
|
I see one of the CI jobs is failing, but it looks like it concerns map, not anything I added |
taterhead247
left a comment
taterhead247
left a comment
There was a problem hiding this comment.
Main thing is that these core endpoints should have anything to do with slack or a particular app.
I only got part way through. But that's the big thing I saw.
| /** | ||
| * Attendance type IDs (from attendance_types table) | ||
| */ | ||
| const ATTENDANCE_TYPE_IDS = { |
There was a problem hiding this comment.
Doesn't our orm know this? Hard coding these seems like a future failure point.
There was a problem hiding this comment.
You're right, I had taken this shortcut in python and I guess the AI picked up on that. I don't expect those types to change anytime soon, but it's worth making future proof
| /** | ||
| * Attendance Router | ||
| * Manages attendance records for event instances. | ||
| * Used by the slackbot for HC/Q/Co-Q operations on preblasts. |
There was a problem hiding this comment.
I know it's just a comment. But I don't think k we should have app-specific language in the generic section.
It is an interesting question to know what apps use what. But I don't think that registry is here.
There was a problem hiding this comment.
Yep, I'll take that out
| schema.users.email, | ||
| ); | ||
|
|
||
| // Get slack user links for each attendee |
There was a problem hiding this comment.
- I don't think anything slack should be in here
- what is this for? Probably something done inside slack?
There was a problem hiding this comment.
Yeah that's fair. I need the Slack user IDs so I can @mention users on Slack and other reasons.
I'm debating whether we could maybe add a query parameter to this like returnSlackIds (default false) or to create a separate endpoint for generic use cases
There was a problem hiding this comment.
I strongly think these core endpoints should be neutral of apps. As basic CRUD as possible I know it'd be somewhat duplicative. But could you copy this one to a slack subfolder and make this one generic?
|
@taterhead247 I made the changes we talked about above |
|
Thanks @evanpetzoldt ! FYI, it looks like this did somehow break typecheck on maps. Something about how description is undefined now. I'm still trying to track down why. I have a PR I'm working on to fix it. I already fixed the lint issue. |
|
ok, it's beyond me. @dnishiyama , can you please check this out locally and run pnpm typecheck and see why Moneyball's changes broke map types? |
evanpetzoldt
force-pushed
the
routes-instance-att-tag
branch
from
7ef49c7 to
d4f6da9
Compare
github-project-automation
bot
moved this from In review
to Merged
in F3 Nation Tech Pull Requests
3 participants
🚧 This PR is Part of the Slackbot rewrite #141
👋 TL;DR
Added routes and tests for:
event-instancesattendanceevent-tagsNote: some of the specific routes (like /v1/event-instance/calendar-home-schedule) is pretty specific to the info I need in slackbot. I could buy an argument that this should live in the slack routes (forthcoming)
(coming soon)
orgchanges (additional fields not currently accessed by routes)orgs->events->event-instances🥜 GIF