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.
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.
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.
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.
Okay, I made it a bit more consistent. I was just trying to use a different name than the start parameter. 1dc178a
|
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 |
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.