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

Metrics hotfix #1572

Merged
merged 2 commits into from
Jul 26, 2022
Merged

Metrics hotfix #1572

merged 2 commits into from
Jul 26, 2022

Conversation

damianhxy
Copy link
Member

Description

  • Don't subtract one when calculating @max_consecutive_assessments
  • Use string instead of symbol to access hash

Motivation and Context

With the recent Rails upgrade, in risk_condition#get_current_for_course, RiskCondition.where(course_id: course_id, version: max_version) now returns a Hash instead of HashWithIndifferentAccess. Hence, we can no longer use symbols interchangeably with strings as keys. This PR modifies the Hash accesses to use string keys.

Furthermore, since consecutive assessments are inclusive, we should not subtract one when calculating @max_consecutive_assessments, else we can't set the grades dropping metric to cover all assignments.

How Has This Been Tested?

Tested workflow for metrics improvement.

Essentially, define categories of 2 assignments to test the following metrics (and considered categories feature)

  • 1 grace day used
  • 50% drop over 2 assignments
  • Did not submit 1 assignment
  • 1 assignment below 51%

Tips and reminders

  • Remember to set max grace days for assignments where you're making a student use grace days
  • Use annotations to award marks (after creating a problem)
  • For drop metric, order is by due_date ASC

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have run rubocop for style check. If you haven't, run overcommit --install && overcommit --sign to use pre-commit hook for linting
  • My change requires a change to the documentation, which is located at Autolab Docs
  • I have updated the documentation accordingly, included in this PR

@damianhxy
Copy link
Member Author

For more context: The Rails 6.0.5 migration resulted in the use of Psych 4, which doesn't allow the storage of ActiveSupport::HashWithIndifferentAccess. Hence in #1552 I called to_hash to store plain Hashes. However, the retrieval code was not changed to accommodate for this, and still attempted to use Symbols interchangeably.

Copy link
Contributor

@victorhuangwq victorhuangwq left a comment

Choose a reason for hiding this comment

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

LGTM. Changes seems to address the issue mentioned

@damianhxy damianhxy merged commit cf105c7 into master Jul 26, 2022
@damianhxy damianhxy deleted the metrics-hotfix branch July 26, 2022 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants