Metrics included/excluded categories#1507
Conversation
|
Note: this change requires rails migration |
|
Tested. It seems like the default behavior for when you resolve and then change the configuration is to archive everything (just like a change of metrics). Is that intended behavior? |
victorhuangwq
left a comment
victorhuangwq
left a comment
There was a problem hiding this comment.
Left some comments with regards to schema.rb. Apart from that tested the functionality of the feature and it works as expected. Looks fantastic btw :)
| t.datetime "resolved_at" | ||
| t.integer "resource_owner_id" | ||
| t.string "access_code" | ||
| t.index ["application_id"], name: "fk_rails_4035c6e0ed" |
There was a problem hiding this comment.
this seems to be a bit odd to push up @ashleyzhang, as this looks like a config?
| ActiveRecord::Schema.define(version: 2022_04_04_193451) do | ||
|
|
||
| create_table "annotations", force: :cascade do |t| | ||
| create_table "annotations", id: :integer, options: "ENGINE=InnoDB DEFAULT CHARSET=utf8mb4", force: :cascade do |t| |
There was a problem hiding this comment.
I think this DB options are MySQL specific. I wonder if there's a way to not push this up?
| t.datetime "created_at", null: false | ||
| t.datetime "revoked_at" | ||
| t.string "scopes" | ||
| t.index ["application_id"], name: "fk_rails_b4b53e07b8" |
There was a problem hiding this comment.
Similarly here, as this looks like a configuration key . Is this a normal thing to do?
| t.datetime "created_at", null: false | ||
| t.string "scopes" | ||
| t.string "previous_refresh_token", default: "", null: false | ||
| t.index ["application_id"], name: "fk_rails_732cb83ab7" |
Yes this is expected! Currently a change in metrics config has the same effect on watchlist instances as a change in risk conditions, i.e. refreshing the list, archiving old instances. |
|
In addition, I've had some misunderstanding of the use of the word "allowlist" and currently allowlist contains all the categories that are not considered in the calculation of metrics. Do we want to change this to blocklist afterwards? |
|
I think that would be confusing - would it be possible to change that in this PR? I'm afraid we might forget that later |
See the latest changes. I rollbacked and changed the relevant migration file instead of writing another migration file for renaming. |
victorhuangwq
left a comment
victorhuangwq
left a comment
There was a problem hiding this comment.
Did a rails db:reset and utilized the new schema. Feature works as expected. Refactor looks fine.
LGTM!
3 participants
Description
Add a configurations tab to metrics feature that includes a "Considered Categories" section enabling professors to decide what assessment categories are or are not included in watchlist calculations.
Motivation and Context
Some courses have optional assessments that do not factor into determining whether or not a student is at risk. Allowing instructors to exclude a category of assessments reduces false positive rates for students appearing in the watchlist.
How Has This Been Tested?
Navigate to the configurations tab in the student metrics dashboard. Exclude a category in the watchlist, and save the changes. Ensure that the change is reflected in the watchlist.
Types of changes
Checklist:
overcommit --install && overcommit --signto use pre-commit hook for lintingOther issues / help required
If unsure, feel free to submit first and we'll help you along.