✨ Add functionality for managing user favorite activities#45
✨ Add functionality for managing user favorite activities#45
Conversation
There was a problem hiding this comment.
Pull request overview
Adds API + service/repository support to let users manage and fetch their favourite activities.
Changes:
- Add endpoints to list favourites (filtered + by user/edition) and to add/remove a favourite.
- Add CQRS query/command methods and service logic to read/update favourite relationships.
- Update repository method to optionally filter “include favourites” queries by
editionId.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| TaleEngine/TaleEngine/Controllers/V1/ActivityController.cs | Adds/updates endpoints for fetching and mutating favourite activities. |
| TaleEngine/TaleEngine.Testing/Controllers/V1/ActivityControllerTests.cs | Adds controller tests for favourites list/add/remove. |
| TaleEngine/TaleEngine.Data/Repositories/ActivityRepository.cs | Updates GetAllIncludeFavs to accept editionId and filter optionally. |
| TaleEngine/TaleEngine.Data.Contracts/Repositories/IActivityRepository.cs | Updates repository contract parameter to editionId. |
| TaleEngine/TaleEngine.Bussiness/Queries/ActivityQueries.cs | Adds FavouriteActivitiesByUserQuery. |
| TaleEngine/TaleEngine.Bussiness/Contracts/IActivityQueries.cs | Exposes FavouriteActivitiesByUserQuery in the queries interface. |
| TaleEngine/TaleEngine.Bussiness/Contracts/IActivityCommands.cs | Exposes add/remove favourite commands. |
| TaleEngine/TaleEngine.Bussiness/Commands/ActivityCommands.cs | Implements add/remove favourite commands via IActivityService. |
| TaleEngine/TaleEngine.Application/ActivityService.cs | Implements favourite list/add/remove logic and updates favourite filtering predicate. |
| TaleEngine/TaleEngine.Application.Contracts/IActivityService.cs | Exposes favourite list/add/remove methods in the service contract. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [HttpPut("[action]/{userId}")] | ||
| public IActionResult GetFavouriteActivitiesFiltered(int userId, [FromBody] ActivityFilterRequest activityFilterRequest) | ||
| { | ||
| int userId = 1; | ||
|
|
||
| var result = _queries.ActiveActivitiesFilteredQuery(activityFilterRequest, userId); | ||
|
|
||
| if (result == null) | ||
| if (result == null || result.Activities == null || result.Activities.Count == 0) | ||
| { | ||
| return NoContent(); | ||
| } | ||
|
|
||
| return Ok(result); | ||
| } |
There was a problem hiding this comment.
There are no controller tests covering the updated GetFavouriteActivitiesFiltered behavior (route now requires userId and the method returns 204 on empty Activities). Adding tests for the 200 and 204 paths would help prevent regressions, especially since this endpoint has different empty-result handling than GetActivitiesFiltered.
| } | ||
|
|
||
| [Fact] | ||
| public void AddFavouriteActivity_Fail_Success() |
There was a problem hiding this comment.
Test name AddFavouriteActivity_Fail_Success is contradictory (it’s a failure-path test expecting 400). Renaming it to reflect the behavior under test (e.g., AddFavouriteActivity_Failure_ReturnsBadRequest) will make the test suite easier to scan and maintain.
| public void AddFavouriteActivity_Fail_Success() | |
| public void AddFavouriteActivity_Failure_ReturnsBadRequest() |
| } | ||
|
|
||
| [Fact] | ||
| public void RemoveFavouriteActivity_Fail_Success() |
There was a problem hiding this comment.
Test name RemoveFavouriteActivity_Fail_Success is contradictory (it’s a failure-path test expecting 400). Consider renaming it to something like RemoveFavouriteActivity_Failure_ReturnsBadRequest for clarity.
| public void RemoveFavouriteActivity_Fail_Success() | |
| public void RemoveFavouriteActivity_Failure_ReturnsBadRequest() |
| if (result == null || result.Activities == null || result.Activities.Count == 0) | ||
| { | ||
| return NoContent(); | ||
| } | ||
|
|
||
| return Ok(result); |
There was a problem hiding this comment.
GetFavouriteActivitiesFiltered returns 204 when result.Activities is empty, but GetActivitiesFiltered returns 200 with an ActivityFilteredResult payload even when the list is empty. Since ActivityFilteredResult includes paging metadata (CurrentPage/TotalPages), returning 204 here drops that information and makes the two filtered endpoints inconsistent. Consider always returning Ok(result) when result != null and letting Activities be empty, or align both endpoints to the same behavior.
| activity = _unitOfWork.ActivityRepository.GetAllIncludeFavs(activity.EditionId) | ||
| .FirstOrDefault(a => a.Id == activityId); | ||
|
|
||
| if (activity == null) | ||
| { | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
AddFavouriteActivity loads the full list of activities (with UsersFav included) for the edition via GetAllIncludeFavs(...) and then filters in-memory with FirstOrDefault. This can become expensive as activities/users grow and is unnecessary when you only need a single activity. Prefer a repository method that fetches a single activity by id with UsersFav included (or change GetAllIncludeFavs to return an IQueryable so the filter runs in SQL).
| activity = _unitOfWork.ActivityRepository.GetAllIncludeFavs(activity.EditionId) | |
| .FirstOrDefault(a => a.Id == activityId); | |
| if (activity == null) | |
| { | |
| return false; | |
| } |
| activity = _unitOfWork.ActivityRepository.GetAllIncludeFavs(activity.EditionId) | ||
| .FirstOrDefault(a => a.Id == activityId); | ||
|
|
||
| if (activity?.UsersFav == null) |
There was a problem hiding this comment.
RemoveFavouriteActivity calls GetAllIncludeFavs(activity.EditionId) (materializing all activities in the edition with favorites) and then filters by id in-memory. This is potentially very costly and can be replaced by a query that fetches just the targeted activity with UsersFav included.
| activity = _unitOfWork.ActivityRepository.GetAllIncludeFavs(activity.EditionId) | |
| .FirstOrDefault(a => a.Id == activityId); | |
| if (activity?.UsersFav == null) | |
| if (activity.UsersFav == null) |
| .Where(a => a.EditionId == editionId | ||
| && a.StatusId == activeStatus.Id |
There was a problem hiding this comment.
GetFavActivitiesFiltered starts from GetAllIncludeFavs(editionId), which already applies an edition filter when editionId is non-default, and then immediately filters a.EditionId == editionId again. This redundancy makes the intent harder to read and risks diverging if repository filtering logic changes. Consider removing the duplicate edition predicate (or, more broadly, pushing the whole filter down into the repository so it runs in the database).
| .Where(a => a.EditionId == editionId | |
| && a.StatusId == activeStatus.Id | |
| .Where(a => a.StatusId == activeStatus.Id |
Add or remove activities from favorites per user