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

Adding paid event routes #27

Open
wants to merge 19 commits into
base: events_ticketing_unification
Choose a base branch
from

Conversation

EthM370
Copy link

@EthM370 EthM370 commented Dec 3, 2024

Adding paid events plugin with get and single attribute update functionality.

@EthM370 EthM370 requested a review from tarashagarwal December 3, 2024 01:07
@EthM370 EthM370 self-assigned this Dec 3, 2024
@EthM370 EthM370 requested review from a team as code owners December 3, 2024 01:07
Copy link
Member

@devksingh4 devksingh4 left a comment

Choose a reason for hiding this comment

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

  1. please write unit tests along with the code
  2. we camel-case our routes so that its consistent - so for example /api/v1/paidEvents/merch - lets try to rewrite those names

Overall good start though!

package-lock.json Outdated Show resolved Hide resolved
src/routes/paidevents.ts Outdated Show resolved Hide resolved
Fixed case issues during git push
Copy link
Author

@EthM370 EthM370 left a comment

Choose a reason for hiding this comment

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

All pending integration and lint issue fixed, still need to write unit test

@tarashagarwal tarashagarwal changed the base branch from main to events_ticketing_unification December 21, 2024 00:10
devksingh4 and others added 12 commits January 5, 2025 14:05
* base setup for group relation creation

* add some stuff

* reorganize codebase to support adding UI

* move all the UI into one repo

* fix git

* test

* fix command

* fix

* fix

* fix deps

* update concurrency groups

* update names

* branch: main for cloudflare
* add group modification route with protected groups support

* modify endpoint names; add listing group membership route

* fix response types

* basic unit tests

* test ui

* fix build dir

* enable better preview

* add panel

* Fix code scanning alert no. 8: Server-side request forgery

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

---------

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
* Handling Undefined process variable while running vite server

* Passing the env varibles in the vite config

* Sourcing ENV Variables from .env at root

* fix definitions

---------

Co-authored-by: tarashagarwal <[email protected]>
Co-authored-by: Dev Singh <[email protected]>
* add basic ui tests

* fix lint

* setup ui tests to be run in our pipeline

* fix health check to run both UI and API

* fix clean command

* fix editorconfig

* fix pipelines to reuse environments and ensure consistency

* fix name

* fix name

* Fix some values
Copy link
Member

@devksingh4 devksingh4 left a comment

Choose a reason for hiding this comment

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

  1. for some reason almost every file was changed? What's going on here? Maybe line endings?
  2. we want to enforce that every parameter, variable, etc is camel case
  3. we should be making our route names "RESTful" where you take an action on a resource (see https://restful-api-design.readthedocs.io/en/latest/ for a decent overview)

Let's talking before/during/after all-hands maybe?

@devksingh4
Copy link
Member

devksingh4 commented Jan 26, 2025

For example, an "event" is fundamentally the resouce, a type of which is "paid". So we should be designing our API to be able to take a given action based on whether it's paid or not in our existing price, not making it a different module as "paidEvent"

There's a nice way to do conditional validation in Zod, where we can say "Hey, if it's a paid event then we need these additional parameters, otherwise we don't. (colinhacks/zod#938 (comment)) I would refine it so that there is a base set of parameters that all needs, and then there is a refined model which is what all paid items need. the overall body is the z.union([base set, refined set]). If some attribute from the refined set exists, then we're creating a paid event. This helps us integrate the paid events setup into our existing events route, without building a new route that will cause confusion by the API consumer.

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.

3 participants