-
Notifications
You must be signed in to change notification settings - Fork 1
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
Initiala ändringar för övergången från Firebase till Expo. #161
base: development
Are you sure you want to change the base?
Conversation
…n av topics sker nu i backend istället
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.
Mina spontana tankar men inväntar @LeoFjatstrom .
Nexpo/Models/NotificationTopics.cs
Outdated
public class NotificationTopic | ||
{ | ||
[DatabaseGenerated(DatabaseGeneratedOption.Identity)] | ||
public int? Id { get; set; } | ||
|
||
[Required] | ||
public TopicType Topic { get; set; } | ||
|
||
[Required] | ||
[JsonIgnore] | ||
public User User { get; set; } | ||
|
||
|
||
|
||
public enum TopicType | ||
{ | ||
All, | ||
Administratior, | ||
CompanyRepresentative, | ||
Student, | ||
|
||
Volunteer, | ||
} |
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.
Jag är lite osäker på vad jag känner inför denna data modelen. Jag ser mycket hellre att vi endast har endast en single source of truth. Så den utgår från användarobjektet för att ta reda på vilka topics användaren borde ta del av. Så att konceptet att "subscribea" till en topic inte kanske finns kvar. (Om det var det som var tanken här?)
Om vi vill låta användaren välja vilka typer av notiser som ska tas emot ser jag hellre två stycken grupper där en kanske är Essential (eller ngt bättre namn) och Marketing. Och att man då i appen kan opta ut från marketing?
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.
tanken är dels att kunna skicka påminnelse notiser till studenter som "subscribat" på events. för detta krävs ny tabell som håller koll på relationen. Håller med om att notiser till särskilda sorters användare inte borde gå till på detta sätt dock.
Tänker vi har borde gå tillväga att vi lägger till/ändrar modeler så vi har
-
notification
- id
- message
- DateTime? scheduledtime
- NotificationType //se nedan
- eventId //nullable men krävs för eventspecifika notiser
- UserNotficiations //länk till användare som får notisen
-
UserNotification - join table för many-to-many relation mellan vilka användare är länkade till vilka notiser
- UserId
- User
- NotificationId
- Notification
-
public enum NotificationType
- general,
- eventReminder
- (specific) ksk för rollbaserade osv
- etc
I User får man även lägga till UserNotifications collection för att representera alla notiser som sagd användare ska få. Via den tabellen kan användaren även hämta dess egna notiser vilket ger history vilket var skevt implementerat innan
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.
Är tanken sen att man ska kunna deep-linka med t.ex eventId i notification
sedan? @LeoFjatstrom
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.
se här för hur man går tillväga med ändringar i databasen också
https://learn.microsoft.com/sv-se/ef/ef6/modeling/code-first/workflows/existing-database
allt med ef6 kan vara viktigt.
för bätte step-by-step utan att bli förvirrade se Update database i readmen
… jointabell. Också ändrat så att token lagras i databasen med användaren. Co-authored-by: pizzaburgare <pizzaburgare@users.noreply.github.com>
@LeoFjatstrom @trilleplay Ni får gärna kika på senaste commiten för att kolla så det ser rimligt ut. Planen är att nästa gång fixa så att man ska kunna pusha notiser med Expo med de tokens vi har lagrat i databasen. |
@@ -7,5 +7,5 @@ public class RegisterUserDTO | |||
public string Token { get; set; } | |||
|
|||
[Required] | |||
public string Topic { get; set; } | |||
public int UserId { get; set; } |
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.
Se: careerfairsystems/nexpo-app#351 (review)
tl;dr, att skicka upp UserId:t såhär, nog inte bästa idéen.
Hanteringen av topics sker nu i backend istället