add daily syncing of LTI rosters #7178

pretendWhale · 2024-08-06T17:00:40Z

Proposed Changes

(Describe your changes here. Also describe the motivation for your changes: what problem do they solve, or how do they improve the application or codebase? If this pull request fixes an open issue, use a keyword to link this pull request to the issue.)

Adds an option to automatically sync student rosters from an external LMS once daily. Note: I did not add tests here, because it would involve ensuring that a schedule was set in redis, which our unit tests do not typically interact with to my knowledge.

Screenshots of your changes (if applicable)

Associated documentation repository pull request (if applicable)

https://github.com/MarkUsProject/Wiki/pull/216

Type of Change

(Write an X or a brief description next to the type or types that best describe your changes.)

Type	Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)
✨ New feature (non-breaking change that adds functionality)	x
🐛 Bug fix (non-breaking change that fixes an issue)
🎨 User interface change (change to user interface; provide screenshots)
♻️ Refactoring (internal change to codebase, without changing functionality)
🚦 Test update (change that only adds or modifies tests)
📦 Dependency update (change that updates a dependency)
🔧 Internal (change that only affects developers or continuous integration)

Checklist

(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the [ ] into a [x] in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)

Before opening your pull request:

I have performed a self-review of my changes.
- Check that all changed files included in this pull request are intentional changes.
- Check that all changes are relevant to the purpose of this pull request, as described above.
I have added tests for my changes, if applicable.
- This is required for all bug fixes and new features.
I have updated the project documentation, if applicable.
- This is required for new features.
If this is my first contribution, I have added myself to the list of contributors.

After opening your pull request:

I have updated the project Changelog (this is required for all changes).
I have verified that the pre-commit.ci checks have passed.
I have verified that the CI tests have passed.
I have reviewed the test coverage changes reported by Coveralls.
I have requested a review from a project maintainer.

Questions and Comments

(Include any questions or comments you have regarding your changes.)

coveralls · 2024-08-08T20:50:13Z

Pull Request Test Coverage Report for Build 10924676601

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

50 of 54 (92.59%) changed or added relevant lines in 8 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.07%) to 91.562%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
config/initializers/config.rb	0	1	0.0%
app/jobs/lti_roster_sync_job.rb	1	4	25.0%

Totals
Change from base Build 10711199044:	0.07%
Covered Lines:	40359
Relevant Lines:	43407

💛 - Coveralls

david-yz-liu

@pretendWhale nice work! I left a few inline comments. It would also be good to add some test cases, e.g. checking that the controller correctly schedules (or removes) the job depending on the automatic_sync option.

david-yz-liu · 2024-08-11T23:22:32Z

app/controllers/courses_controller.rb

+    job_args[:role_types] = roles
+    job_args[:can_create_users] = allowed_to?(:lti_manage?, with: UserPolicy)
+    job_args[:can_create_roles] = allowed_to?(:manage?, with: RolePolicy)
+    name = "#{@current_course.name}_#{root_path.tr!('/', '')}"


I assume this is just a unique identifier for the job? If so, I think using the deployment id (in addition to root_path) is safer, since I believe we allow courses to have multiple deployments.

Also adding the name of the class LtiRosterSyncJob would be good in case we want to do more scheduling in the future.

david-yz-liu · 2024-08-11T23:22:52Z

app/controllers/courses_controller.rb

-                                                  can_create_users: allowed_to?(:lti_manage?, with: UserPolicy),
-                                                  can_create_roles: allowed_to?(:manage?, with: RolePolicy))
+    job_args = {}
+    job_args[:deployment] = deployment.id


Name this deployment_id (consistent with other, similar params)

david-yz-liu · 2024-08-11T23:30:04Z

app/controllers/courses_controller.rb

+      config[:args] = { job_class: 'LtiRosterSyncJob', arguments: [job_args] }
+      config[:cron] = Settings.lti.sync_schedule
+      config[:persist] = true
+      config[:queue] = 'DEFAULT_QUEUE'


This doesn't seem like it should be hard-coded. Can we use the LtiRosterSyncJob.queue_name method?

david-yz-liu · 2024-08-11T23:31:54Z

app/jobs/lti_roster_sync_job.rb

+  def perform(args)
+    args = args.deep_symbolize_keys
+    lti_deployment = LtiDeployment.find(args[:deployment])
+    course = lti_deployment.course


It seems like defining a course variable is redundant here; can you modify roster_sync so that it doesn't take in a course argument?

david-yz-liu · 2024-08-11T23:47:37Z

config/initializers/config.rb

@@ -193,6 +193,7 @@
        required(:domains).array(:str?)
        required(:token_endpoint).filled(:string)
        optional(:unpermitted_new_course_message).filled(:string)
+        optional(:sync_schedule).filled(:string)


I think this should be required, otherwise the automatic sync functionality will fail.

david-yz-liu · 2024-08-11T23:48:08Z

config/locales/common/en.yml

@@ -48,6 +48,7 @@ en:
    select_course: Select the course that matches %{course_name}
    start_grade_sync: Syncing Grades
    start_roster_sync: Syncing Roster
+    sync_automatically: Sync Automatically


Reword to "Enable automatic syncing"

for more information, see https://pre-commit.ci

david-yz-liu · 2024-09-18T02:51:07Z

Changelog.md

@@ -42,6 +42,7 @@
 - Calculate and display the exact future time for the next token generation to help students plan their test runs more effectively. (#7127)
 - Set the default date for "Tokens available on" to the current time (#7173).
 - Add setting to enable filtering of new course creation from LTI launch (#7151)
+- Add daily roster syncing via LTI (#7178)


This entry is in the wrong section

You are absolutely correct - thanks for the catch!

pretendWhale · 2024-09-18T15:26:21Z

Failing test seems to be a flaky test unrelated to the changes in this PR (check_repo_permissions test failing)

david-yz-liu · 2024-09-18T17:07:24Z

@pretendWhale while this might be a flaky test, I've now re-run the tests twice and they failed both additional times. This is suspicious. Can you please confirm that this test is passing for you locally on this branch?

pretendWhale · 2024-09-18T18:41:12Z

@david-yz-liu that is strange. I can confirm that this test (and all tests in spec/db/check_repo_permissions_spec.rb) pass when I run them on this branch locally. I ran them 3 times and did not see any failures.

pretendWhale added 3 commits August 6, 2024 12:59

add daily syncing of LTI rosters

0959852

use schedule specified in settings

d8fea56

changelog: update changelog

9e54a0d

pretendWhale marked this pull request as ready for review August 8, 2024 20:20

pretendWhale requested a review from donny-wong August 8, 2024 20:21

Merge branch 'master' into automatic_roster_sync

1fcb3bd

Merge branch 'master' into automatic_roster_sync

efebbb5

donny-wong approved these changes Aug 9, 2024

View reviewed changes

donny-wong requested a review from david-yz-liu August 9, 2024 21:26

david-yz-liu reviewed Aug 11, 2024

View reviewed changes

donny-wong added this to the v2.5.1 milestone Aug 13, 2024

pretendWhale and others added 2 commits August 13, 2024 15:38

updates in response to PR feedback

4f67126

[pre-commit.ci] auto fixes from pre-commit.com hooks

b9acf1a

for more information, see https://pre-commit.ci

pretendWhale modified the milestones: v2.5.1, v2.5.2 Aug 27, 2024

pretendWhale added 3 commits September 12, 2024 18:14

Merge branch 'master' into automatic_roster_sync

cc8873d

tests: lti roster sync tests

d95b539

do not use hardcoded root path in tests

05f019c

pretendWhale requested a review from donny-wong September 13, 2024 20:21

tests:add test

7c45df8

donny-wong approved these changes Sep 17, 2024

View reviewed changes

donny-wong requested a review from david-yz-liu September 17, 2024 19:54

david-yz-liu reviewed Sep 18, 2024

View reviewed changes

move changelog entry to appropriate place

3842953

pretendWhale requested a review from david-yz-liu September 18, 2024 14:52

david-yz-liu approved these changes Sep 18, 2024

View reviewed changes

david-yz-liu merged commit 12ac171 into MarkUsProject:master Sep 18, 2024
6 checks passed

pretendWhale added a commit to pretendWhale/Markus that referenced this pull request Sep 25, 2024

Enable cron-based automatic syncing of LTI rosters (MarkUsProject#7178)

955e3a2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add daily syncing of LTI rosters #7178

add daily syncing of LTI rosters #7178

pretendWhale commented Aug 6, 2024 •

edited

Loading

coveralls commented Aug 8, 2024 •

edited

Loading

david-yz-liu left a comment

david-yz-liu Aug 11, 2024

david-yz-liu Aug 11, 2024

david-yz-liu Aug 11, 2024

david-yz-liu Aug 11, 2024

david-yz-liu Aug 11, 2024

david-yz-liu Aug 11, 2024

david-yz-liu Sep 18, 2024

pretendWhale Sep 18, 2024

pretendWhale commented Sep 18, 2024

david-yz-liu commented Sep 18, 2024

pretendWhale commented Sep 18, 2024

add daily syncing of LTI rosters #7178

add daily syncing of LTI rosters #7178

Conversation

pretendWhale commented Aug 6, 2024 • edited Loading

Proposed Changes

Type of Change

Checklist

Questions and Comments

coveralls commented Aug 8, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10924676601

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

david-yz-liu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pretendWhale commented Sep 18, 2024

david-yz-liu commented Sep 18, 2024

pretendWhale commented Sep 18, 2024

pretendWhale commented Aug 6, 2024 •

edited

Loading

coveralls commented Aug 8, 2024 •

edited

Loading