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

feature/ja-115 AvailableScheduleForAd - Fix #104

Merged

Conversation

skrawus
Copy link
Collaborator

@skrawus skrawus commented Jun 2, 2024

I fixed AvailableScheduleForAd ViewComponent. Till now it listed all the ScheduleItemRequests that Student sent to any Ad.
Now it shows only the requests sent to the same Ad the Student's looking at.

Copy link
Collaborator

@Zjyslav Zjyslav left a comment

Choose a reason for hiding this comment

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

It's a good thing to fix, but a wrong place to do it.
The class backing the view component, just like controllers for views, should include as little logic as possible - it should assemble requests to services and display responses.
You shouldn't do it here, because the problem may be fixed for this particular partial view, but we still have broken business logic in our service and we cannot use it reliably anywhere else without adding this fix there as well.
Please move the .Where you applied to the response.Items to the query in StudentService.GetAvailableScheduleForAd().

@skrawus
Copy link
Collaborator Author

skrawus commented Jun 7, 2024

Thanks! Done.

Copy link
Collaborator

@Zjyslav Zjyslav left a comment

Choose a reason for hiding this comment

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

Great!

@skrawus skrawus merged commit 8ccd706 into develop Jun 9, 2024
1 check passed
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.

2 participants