Skip to content

Lego/feat/coupons#317

Merged
joseglego merged 3 commits intomainfrom
lego/feat/coupons
Nov 8, 2024
Merged

Lego/feat/coupons#317
joseglego merged 3 commits intomainfrom
lego/feat/coupons

Conversation

@joseglego
Copy link
Copy Markdown
Member

@joseglego joseglego commented Nov 8, 2024

Summary

  • Define Coupons table with its dependence on Event and with its relation with Tickets
  • include query/filter on Events for tickets with coupons.

@joseglego joseglego requested review from TextC0de and fforres November 8, 2024 05:35
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Nov 8, 2024

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 86.13% 19898 / 23101
🔵 Statements 86.13% 19898 / 23101
🔵 Functions 79.81% 510 / 639
🔵 Branches 80.67% 1549 / 1920
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
src/datasources/db/coupons.ts 100% 100% 100% 100%
src/datasources/db/schema.ts 100% 100% 100% 100%
src/datasources/db/tickets.ts 100% 100% 100% 100%
src/schema/events/types.ts 78.27% 80.76% 66.66% 78.27% 139-144, 149-155, 161-174, 180-193, 199-212, 218-231, 280-286, 368-370, 387-397, 418-427, 446-447, 481-484
src/schema/ticket/ticketsFetcher.ts 86.13% 75% 66.66% 86.13% 60-61, 82, 117-132
Generated in workflow #1343 for commit 4fa92d1 by the Vitest Coverage Report Action

(t) => ({
eventIdIndex: index("coupons_event_id_index").on(t.eventId),
codeIndex: index("coupons_code_index").on(t.code),
uniqueEventCodeIndex: uniqueIndex("coupons_event_code_unique_index").on(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

<3

if (couponId && couponId.length > 0) {
wheres.push(eq(ticketsSchema.couponId, couponId));
} else {
wheres.push(isNull(ticketsSchema.couponId));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

es necesario este isNull?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Creo que esto haría que todos los tickets sin cupon no se muestren o no?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sip! La idea es que si no tienes un coupon válido. SOLO devuelve tickets publicos (sin coupon)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Lo veo bien 👍

Comment thread src/datasources/db/coupons.ts Outdated
eventIdIndex: index("coupons_event_id_index").on(t.eventId),
codeIndex: index("coupons_code_index").on(t.code),
uniqueEventCodeIndex: uniqueIndex("coupons_event_code_unique_index").on(
t.eventId,
Copy link
Copy Markdown
Collaborator

@TextC0de TextC0de Nov 8, 2024

Choose a reason for hiding this comment

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

Creo que el código no deberia ser unico por evento, sino que por ticket.

Por ejemplo, para 9punto5 teniamos dos entradas:

  • Experiencia
  • Conferencia

Y en ambos tickets se debía poder usar un mismo código

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sip, esta implementación te permite esto. Al crear el código A para 9punto5, puedes tener 2 tickets, cada ticket debe tener este código A.id como couponId. y listo.

Es tal cuál como comentas. tenemos many tickets y one event en la definición de relations

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ok, entiendo, tienes razon que lo permite 👍

.notNull(),
stripeProductId: text("stripe_product_id"),
mercadoPagoProductId: text("mercado_pago_product_id"),
couponId: uuid("coupon_id").references(() => couponsSchema.id),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Un mismo ticket debería poder tener varios cupones de descuento asociados.
Cada uno de estos cupones podría aplicar distintos montos de descuento.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Igual entiendo que esto es para un MVP similar a lo de 9punto5 pero mejor jajja.
Entonces me imagino van a crear un ticket por cada cupón?

No se como lo iban a implementar pero si es para el MVP tan solo un campo "mvpCoupon" de text podría servir creo por ahora???

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

En general, respondiendo tu comentario y comentario de comentario 😆

Sip, es la reimplementación de los de 9punto5, (digamos que 9punto5 es v1, esto es V1.1) creo que nos queda mucho por iterar. La idea de reusar eso es para que así la lógica del purchaseOrder continuará sin ningún cambio. Me pareció lo más sencillo por limitaciones de tiempo y que era el approach más económico.

Sip, yo creería que más que mvp es v1.1. No agregué el prefix para que no lleguemos a versión final_final_este_si_de_verdad 🤣

if (couponId && couponId.length > 0) {
wheres.push(eq(ticketsSchema.couponId, couponId));
} else {
wheres.push(isNull(ticketsSchema.couponId));
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Creo que esto haría que todos los tickets sin cupon no se muestren o no?

@joseglego joseglego merged commit 2c2dbf6 into main Nov 8, 2024
@joseglego joseglego deleted the lego/feat/coupons branch November 8, 2024 07:12
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