Error messages for generating schedules #423
Conversation
* Also removes a print statement left from debugging
Other ideas for the no schedules possible text:
What does everyone think? I think these are clear enough to tell the user both what went wrong and how to fix it. |
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.
will review this again, but some minor comments I had the other day
I think that was separate PR |
Correct, we don't have a mockup for a new design and I wanted this PR to be focused on the functionality itself rather than the styling of how we display it |
Also anytime it mentions a specific section as the reason, you should bold the section text so you immediately focus on it. |
Do you still want me to implement this? We don't post-process text anywhere else so I'm wondering how you want me to go about doing it
Since you could either add more sections or remove busy times to fix the issue in most cases and narrowing it down more than that is impossible unless all sections are selected for all classes on your schedule, I'm not going to implement this I'd also appreciate feedback from someone on what to change the generic error message to (see my earlier comment) |
If it's possible yeah, I think it would be beneficial
|
I like how the second half gives suggestions for how to fix it, but the first part feels a little confrontational to me. I would try |
Agreed, definitely cause the usage of
I think |
I don't think it's that helpful and I'm not sure how I would go about doing it, so for now I'm going to leave it as-is. We can make it into a separate issue if we decide that we want it to work like that, but I don't want it to hold up this getting merged before launch Also I changed the generic message to: So if we agree the bold text isn't a big enough deal to prevent this from getting merged this should be ready for review |
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.
Didn't do any testing just looked at code, so will look at this again later today (hopefully)
// Fetches scheduler/generate. If something goes wrong or no schedules can be generated, | ||
// throws an error with a message indicating what happened. |
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.
// Fetches scheduler/generate. If something goes wrong or no schedules can be generated, | |
// throws an error with a message indicating what happened. | |
/** Fetches scheduler/generate. If something goes wrong or no schedules can be generated, | |
throws an error with a message indicating what happened. */ |
So it shows up as a doc when you hover over the function
'{subject} {course_num}: None of the sections you selected are compatible with your ' | ||
'available times. Either select more sections, or remove some of your busy times.' |
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.
Should we be switching between available
and busy
here? I get why you did it but just wanted to double check
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.
Yeah I was thinking about that as I wrote this, and it doesn't really make sense if it uses one or the other so I left it like this
)) | ||
return sections |
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.
nitpick lol
)) | |
return sections | |
)) | |
return sections |
'schedules': [[SectionSerializer(section).data for section in self.sections]], | ||
'message': '', | ||
} | ||
print(expected) |
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.
print(expected) |
Also need to merge master into this, presumably b/c #417 |
Changes made except the chained |
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.
Description
Adds messages describing what went wrong when generate schedules doesn't return any schedules. I'd appreciate if everyone could try to think of other reason generating schedules might fail so we can be sure these error messages are accurate and cover everything that could possibly go wrong.
Rationale
There are a couple assumptions I make to avoid making extra queries just to check if sections still exist in the queryset:
With these assumptions, these are the three ways generating schedules can fail and their messages:
There's also the possibility that the call fails (since the server is down or the schedules are malformatted), for this case I added another message to the frontend saying that something went wrong.
I also refactored the section filters on the frontend to use an enum for consistency, since it's always bothered me that we use explicit values and this PR is vaguely related to section filters.
How to test
Try to generate schedules with various constraints that may or may not result in no schedules being generated. I think I was pretty thorough with the ways the call can fail, but there's always the possibility I missed something.
Screenshots
Previous error message:
Example new error messages:
Related tasks
Closes #399.