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

feat: mark events as favorites #284

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

Conversation

maakinoh
Copy link
Contributor

@maakinoh maakinoh commented Feb 9, 2025

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.

Copy link
Member

@Metawolve Metawolve left a 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.

@maakinoh
Copy link
Contributor Author

Good point, your approach would be more clean.

I wasn't quite sure if the UserRecord was still in active use, apart from the TG bot, and actively filled with data.
But I think it shouldn't be an issue to add the users' IDP Id to this table then, right? Dunno if this approach would mess somewhere.

@maakinoh maakinoh requested a review from Rain336 February 14, 2025 08:22
Copy link
Member

@Metawolve Metawolve left a 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

Copy link
Member

@Metawolve Metawolve left a comment

Choose a reason for hiding this comment

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

Lgtm

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.

Making Event favorites a server-side feature
2 participants