Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upEducators import: Mark `missing_from_last_export` and show project leads in permissions tools #2546
Conversation
| interventions: Field::HasMany.with_options(searchable: false), | ||
| id: Field::Number.with_options(searchable: false), | ||
| email: Field::String.with_options(searchable: false), | ||
| encrypted_password: Field::String.with_options(searchable: false), |
This comment has been minimized.
This comment has been minimized.
kevinrobinson
Aug 23, 2019
•
Author
Contributor
These seem scary but most haven't existed on the model in a long time, and none of these extra fields should have had any impact on what data was exposed to project leads in the UI anyway. Without digging into Administrate all the way, these appear to be only limiting what options are available for other config, based on how this behaved editing them here.
That being said, another audit pass on administrate would be a good security step.
This comment has been minimized.
This comment has been minimized.
|
selfie |
kevinrobinson
merged commit edf2ca5
into
master
1 check was pending
1 check was pending
This was referenced Aug 23, 2019
This comment has been minimized.
This comment has been minimized.
|
Done this migration, verified across districts. |
This was referenced Aug 23, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
kevinrobinson commentedAug 23, 2019
•
edited
Who is this PR for?
Project leads across districts
What problem does this PR fix?
Typically project leads or district IT staff remove educators from the authentication system (eg, LDAP), which means they are not allowed to use Student Insights at all, regardless of past permissions.
By design, the district LDAP system controls who has access regardless of any data within Student Insights. But Student Insights stores records for staff and students after they have been removed from the daily export, so that it can maintain past information (eg, who wrote a note about a student).
For educator records, this distinction isn't tracked or shown. One of the project leads asked about it for an older staff. They understandably were concerned that seeing their name meant that person still had access.
What does this PR do?
Updates the importer and educator model to track this with
missing_from_last_export, similar to theStudentsImporter. It also backports a fix for an edge case that hasn't come up to theStudentsImporter.Uses this info in permissions tools for project leads so this process is visible for them. Removes some old cruft from the administrate config.
This PR doesn't update the authentication process, but a separate one will add a defensive layer beyond the district's own authentication check that ensures the educator record was still present in the latest SIS export.
Screenshot (if adding a client-side feature)
with the full blinding beauty of adminstrate forms :)
overview
adjust permissions list
view
edit
Checklists
Which features or pages does this PR touch?
Does this PR use tests to help verify we can deploy these changes quickly and confidently?