-
Notifications
You must be signed in to change notification settings - Fork 1
Fix create events in control panel #2013
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
base: master
Are you sure you want to change the base?
Conversation
frontend/src/utils.ts
Outdated
| default: { | ||
| const _exhaustiveCheck: never = ticketType; | ||
| return _exhaustiveCheck; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Denne biten er unødvendig, compileren vil uansett advare om ikke alle verdier av EventTicketType er sjekket her
frontend/src/api.ts
Outdated
| } | ||
|
|
||
| export async function getBilligEvents(): Promise<BilligEventDto[]> { | ||
| const BILLIG_EVENT_LIST_URL = '/api/billig-event/'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bør ikke hardkode URLer her, bruk heller const'ene definert i src/routes/backend.ts
frontend/src/api.ts
Outdated
| } | ||
|
|
||
| export async function getActiveBilligEvents(): Promise<BilligEventDto[]> { | ||
| const BILLIG_EVENT_LIST_URL = '/api/billig-event/'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Samme som for getBilligEvents
| const BILLIG_EVENT_LIST_URL = '/api/billig-event/'; | ||
| const url = `${BACKEND_DOMAIN}/${BILLIG_EVENT_LIST_URL}`; | ||
| const response = await axios.get<BilligEventDto[]>(url, { withCredentials: true }); | ||
| return response.data.filter((e) => e.in_sale_period && !e.is_sold_out); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Liiiitt guffent at frontend skal måtte filtrere her. Dette bør gjøres av backend i et eget endepunkt (eller eget valgfritt filter man sender med til endepunktet). Om det ikke allerede er gjort, trengs det ikke å gjøres i PRen her, men sleng inn en TODO om det her og lag en issue på det i linear
| id: z.number(), | ||
| name_nb: z.string().min(1, 'Name (Norwegian) is required'), | ||
| name_en: z.string().min(1, 'Name (English) is required'), | ||
| price: z.number({ invalid_type_error: 'Price must be a number' }).min(0, 'Price must be at least 0'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prosjektet bruker zod-i18n så det skal ikke være nødvendig å definere egen feilmelding på "ikke-custom" sjekker slik som min
| <FormItem> | ||
| <FormLabel>{t(KEY.event_registration_url)}</FormLabel> | ||
| <FormControl> | ||
| <input {...field} placeholder="http://..." /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bruk Input komponenten, og husk type="text"
| <input {...field} placeholder="http://..." /> | |
| <Input type="text" {...field} placeholder="http://..." /> |
frontend/src/i18n/translations.ts
Outdated
| [KEY.common_ticket_type_free]: 'Gratis', | ||
| [KEY.common_ticket_type_free_with_registration]: 'Gratis med registrering', | ||
| [KEY.common_ticket_type_custom]: 'Tilpasset', | ||
| [KEY.common_ticket_type_billig]: 'Betalt', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vet at PRen her ikke har rørt det her, men syns vi i samme slengen kan endre fra "Betalt" til "Billig".
Det vil kun være forvirrende for de som er vant med å lage arrangementer på samf3 at Billig ikke er et alternativ i dropdownen. Om de ser Billig så vet de hva det betyr, det samme gjelder ikke for "Betalt"
frontend/src/schema/event.ts
Outdated
| export const EVENT_END_DT = z.string().optional(); | ||
| export const EVENT_HOST = z.string().min(1, { message: 'Arrangør er påkrevd' }); | ||
| export const EVENT_LOCATION = z.string().min(1, { message: 'Lokale er påkrevd' }); | ||
| export const EVENT_CAPACITY = z.number().min(1, { message: 'Kapasitet må være større enn 0' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Capacity burde ikke være påkrevd. Har nettopp merget en PR som fikser det slik at capacity kan være NULL i databasen, så event formen må oppdateres for å reflektere det.
Kan tenkes at vi for enkelte event types krever å sette capacity, f.eks. registration, men det kan vi gjøre ved et senere tidspunkt, ikke viktig nå.
| ...values, | ||
| visibility_to_dt: computedEndDt ? computedEndDt.toISOString() : '', | ||
| end_dt: computedEndDt ? computedEndDt.toISOString() : '', | ||
| } as unknown as EventDto; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dette er litt typescript fy-fy hehe...
Foreslår at vi heller endrer typen til payload til Partial<EventDto> og endre parameter-typen til postEvent til det samme, og så lar vi heller backenden deale med resten.
| duration: number; | ||
| end_dt: string; | ||
| publish_dt: string; | ||
| visibility_from_dt: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mangler også visibility_to_dt
| visibility_from_dt: string; | |
| visibility_from_dt: string; | |
| visibility_to_dt: string; |
| @@ -0,0 +1,18 @@ | |||
| # Generated by Django 5.1.9 on 2025-12-16 16:17 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Migration-filen her må slettes og kjør makemigrations på nytt pga PRen som jeg merget, sorri 🙏
No description provided.