-
Notifications
You must be signed in to change notification settings - Fork 5
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
feat: mark events as favorites #284
base: main
Are you sure you want to change the base?
Conversation
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.
This looks like a reasonable solution.
But maybe a suggestion - not sure which way is actually better, both would be fine for me - but another way would be to have a direct n:m relationship between users and events. Meaning that the EventRecord would have a new property that is a list of users (e.g. "FavoredBy") and the UserRecord will have a new property that is a list of events (e.g. "FavoriteEvents"). The names are just suggestions, there's probably a better way to name them.
But this would mean that you wouldn't need an EventFavoriteRecord class, as EF Core will handle this automatically and create a relation table in the database (probably called UserRecordsEventRecords or something, although you can customize the name in the modelbuilder).
You then can just add events to favorites by just adding them to the list of FavoriteEvents of the user, and remove them by just removing them from that list.
Then you also could easily get the list of users that marked an event as favorite, by just getting event.FavoredBy, e.g. to show who favored an event in the app, if we want to do that in the future.
Good point, your approach would be more clean. I wasn't quite sure if the |
…entity table was renamed to Users (and their corresponding records classes). Another class UserRecord was also renamed to UserResponse
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.
Looks good apart from some minor comments
src/Eurofurence.App.Server.Services/Events/EventFavoritesService.cs
Outdated
Show resolved
Hide resolved
src/Eurofurence.App.Server.Services/Events/EventFavoritesService.cs
Outdated
Show resolved
Hide resolved
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.
Lgtm
Closes: #276.
This PR introduces new endpoints and a database table for marking events as favorites.
The nice to have feature about 'sharing favorite events' is probably best handled in a separate PR.