Skip to content

AA-393: Update Next Section Logic - #25485

Merged
Dillon-Dumesnil merged 1 commit into
masterfrom
ddumesnil/weekly-highlights-error-logging-aa-393
Nov 2, 2020
Merged

AA-393: Update Next Section Logic#25485
Dillon-Dumesnil merged 1 commit into
masterfrom
ddumesnil/weekly-highlights-error-logging-aa-393

Conversation

@Dillon-Dumesnil

Copy link
Copy Markdown
Contributor

Inside content_highlights.py, we had code to calculate due dates
for when there isn't graded content, but we could only reach that
code path if the user had an assignment with a due date at the
target date. Now we will check for all learners who could be in
range of having an update and let the code in content_highlights.py
decide if a highlight should be sent

Comment on lines -227 to -229
# Exclude deprecated fields
exclude = ('dashboard_tracking_code',)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Realized I forgot this during earlier clean-up

@mikix mikix left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this makes sense to me.

@Dillon-Dumesnil
Dillon-Dumesnil force-pushed the ddumesnil/weekly-highlights-error-logging-aa-393 branch 2 times, most recently from 2adbd45 to cb3c990 Compare October 30, 2020 15:05
Comment on lines 482 to 486

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Now that we log out the more descriptive message, I felt like I could remove this. Maybe I should use a prefix though so they are easy to find. Let me update.

Comment on lines 172 to 174

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed this and switched to return None, None because now that we check all schedules within a certain time period, this scenario is going to happen very often and logging it out would be unhelpful.

Comment on lines 189 to 188

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We manually calculate due dates in our function anyway so these weren't doing anything

@Dillon-Dumesnil
Dillon-Dumesnil force-pushed the ddumesnil/weekly-highlights-error-logging-aa-393 branch 3 times, most recently from 335feb4 to a8eadb5 Compare October 30, 2020 18:09

@ciduarte ciduarte left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for adding in those doc strings!

Inside content_highlights.py, we had code to calculate due dates
for when there isn't graded content, but we could only reach that
code path if the user had an assignment with a due date at the
target date. Now we will check for all learners who could be in
range of having an update and let the code in content_highlights.py
decide if a highlight should be sent
@Dillon-Dumesnil
Dillon-Dumesnil force-pushed the ddumesnil/weekly-highlights-error-logging-aa-393 branch from a8eadb5 to 6b16fcd Compare October 30, 2020 19:02
ItemFactory.create(parent=self.course, category='chapter', highlights=['good stuff 1'])
ItemFactory.create(parent=self.course, category='chapter', highlights=['good stuff 2'])
ItemFactory.create(parent=self.course, category='chapter', highlights=['good stuff 3'])
ItemFactory.create(parent=self.course, category='chapter', highlights=['good stuff 4'])

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So I added in this fourth one because the course duration has the default value of 28 days. So by having 4 sections, the offsets will be evenly divided instead of possibly having something like 9 days and 8 hours (which is what it was when there were only three)

@edx-status-bot

Copy link
Copy Markdown

Your PR has finished running tests. There were no failures.

@Dillon-Dumesnil
Dillon-Dumesnil merged commit 9ebcf32 into master Nov 2, 2020
@Dillon-Dumesnil
Dillon-Dumesnil deleted the ddumesnil/weekly-highlights-error-logging-aa-393 branch November 2, 2020 13:36
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

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.

5 participants