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 110 students schedule is remote #105

Merged
merged 13 commits into from
Jun 16, 2024

Conversation

skrawus
Copy link
Collaborator

@skrawus skrawus commented Jun 2, 2024

I have added IsRemote to CreateScheduleItemRequest
Additionally I wrote some tests to StudentService.

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.

I think some things got mixed up in the UI part.
What we needed to add was to let student set value of IsRemove of ScheduleItemRequest they want to create.
It's separate matter from IsRemote of the Ad.

I think it's still good you added IsRemote to the AvailableScheduleForAdResponse, but instead of what you did, I'd use it to display (or enable/disable) the IsRemote checkbox or not (as in:

  • if Ad.IsRemote is true, ScheduleItemRequest.IsRemote can be true or false,
  • but if Ad.IsRemote is false, we want ScheduleItemRequest.IsRemote to always be false.

We could put a guard for it in StudentService).

With that in mind, instead of what you added to Default.cshtml, I'd recommend to just add <input name="requestIsRemote" type="checkbox"....
Don't use asp-for, because this form doesn't match the model of this view component - we want to set ScheduleItemRequest.IsRemote not model.IsRemote which is Ad.IsRemote.
Then, you can do some @if(model.IsRemote) to not allow student to set requestIsRemote if model.IsRemote is false.

Other than that, I had some comments to the tests and I'd also recomment to combine first 2 tests into one [Theory] with 2 versions of input. I see no reason to treat them as separately named cases - both are a case of valid input, just with different values and you check it the method actually uses the value you provide. Also, if you make the changes I requested, you won't have to make them twice.

As to what you changed in the StudentService, I have no objections. Looks absolutely correct.

skrawus added 6 commits June 14, 2024 16:04
CreateScheduleItemRequest_WhenIsRemoteIsTrue_ShouldSetIsRemoteToTrue
CreateScheduleItemRequest_WhenIsRemoteIsTrue_ShouldSetIsRemoteToTrue
CreateScheduleItemRequest_WhenRequestSent_ShouldReturnSuccess
CreateScheduleItemRequest_ShouldSetIsRemoteCorrectly
@skrawus
Copy link
Collaborator Author

skrawus commented Jun 15, 2024

Okay, thanks. I think I've got everything as requested.

@skrawus skrawus requested a review from Zjyslav June 15, 2024 10:31
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 turns out that getting a bool value from a checkbox is a bit tricky. If you can bind checkbox to a property in your model, it works, but when you just try to read its value, it's not true, but rather on, which doesn't translate nicely and gets you a default false every time.

I have one idea how to solve this - move the form to a partial view, bind it to something like:

namespace TutorLizard.Web.Models;
public class CreateScheduleItemRequestViewModel
{
    public int ScheduleItemId {get; set;}
    public int AdId {get; set;}
    public bool RequestIsRemote {get; set;}
    public bool AdIsRemote {get; set;}
}

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 works.

@Zjyslav Zjyslav merged commit 2d5dc59 into develop Jun 16, 2024
1 check passed
@Zjyslav Zjyslav deleted the feature/ja-110_students-schedule-IsRemote branch June 16, 2024 16:48
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