-
Notifications
You must be signed in to change notification settings - Fork 38
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
Use course_date_range
logic for GraphQL course start date (#1430)
#1431
Use course_date_range
logic for GraphQL course start date (#1430)
#1431
Conversation
This is how I was thinking of changing it. I'd might just remove the I also think we should have these fields a different name than get_date_start on the model. I'm not sure what I'd call it. It's really also doing a |
if self.date_start is not None: | ||
start = self.date_start | ||
elif self.term is not None and self.term.date_start is not None: | ||
start = self.term.date_start | ||
else: | ||
logger.info(f"No date_start value was found for course {self.name} ({self.canvas_id}) or term; setting to current date and time") | ||
start = datetime.now(pytz.UTC) | ||
return start | ||
|
||
def determine_date_end(self, start: Union[datetime, None] = None) -> datetime: |
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.
Maybe passing the start here is too fancy, but maybe it keeps things more consistent and saves a query in some circumstances.
Scenarios to consider:
- No course dates, no term dates, current time and two weeks from now; better synchronized but probably only by milliseconds, which is basically irrelevant.
- No date end, no term dates, but a course start date. Query for course doesn't need to be re-made when getting the course start?
These may not matter much anyway, since I tried accessing a course without a term (which I manually deleted), and it said that course data was still being processed.
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.
I was able to test these scenarios and I was able to get correct result.
In case of no end date, Things were fine as well. I don't think Assignment planning relies on end date. But resource access view does and in that case the bar was showing 2 weeks only. The behavior is same from before. All this change was doing is name change of the function.
I am reviewing it now. |
I am running into some setup issues, so I have a delay in reviewing. But I don't see those issues as part of this PR. |
dashboard/models.py
Outdated
"setting to two weeks past start date." | ||
) | ||
date_start = start if start else self.determine_date_start() | ||
end = date_start + timedelta(weeks=2) |
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.
I felt the assignment of the end date could be done with date_end
variable if you like as it is done for date_start
just for consistency.
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.
Okay, I made it a bit more consistent. I was just trying to use a different name than the start
parameter. 1dc178a
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.
Nice!
One think I noticed is when It is a moot point but I feel we could populate that course start and end date in response (since we are calculating it based on our ideas and AP/RA view use that value as well )for clarity purposes. But this is nothing related to this issue |
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.
I see this change as an improvement and working as expected. I have some comment but are optional
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.
The date lookup logic looks good to me. Test was a success.
Thanks for noting this @pushyamig. I think it would be worth examining that view again to make sure we're returning consistent data that is actually used. I don't see us using these course dates or the term really from that particular endpoint in the frontend. Maybe we did at some point. I think for now we should be okay. I've created a separate issue to look into this later: #1433 |
…mich-edu#1430) (tl-its-umich-edu#1431) * Add first draft of fix for GraphQL course start date * Rename methods; fix determine_date_end; tidy up * Use determine_date_start in views * Remove course_date_range * Reorganize imports * Change some variable names * Move import
This PR aims to resolve #1430.