Use DB configured canvas assignments in launches #3127
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
marcospri
force-pushed
the
ext_lti_assignment_id-column-launches
branch
from
702f42b
to
a2e93ea
Compare
marcospri
force-pushed
the
ext_lti_assignment_id-column-frontend
branch
2 times, most recently
from
057dae5
to
48a4c00
Compare
This was referenced Sep 20, 2021
marcospri
force-pushed
the
ext_lti_assignment_id-column-frontend
branch
2 times, most recently
from
39f14dc
to
d6b01f8
Compare
marcospri
force-pushed
the
ext_lti_assignment_id-column-launches
branch
from
a2e93ea
to
a7a096a
Compare
marcospri
force-pushed
the
ext_lti_assignment_id-column-frontend
branch
from
d6b01f8
to
d1f91f9
Compare
marcospri
force-pushed
the
ext_lti_assignment_id-column-launches
branch
from
a7a096a
to
ff32a99
Compare
marcospri
force-pushed
the
ext_lti_assignment_id-column-frontend
branch
from
d1f91f9
to
196834c
Compare
marcospri
force-pushed
the
ext_lti_assignment_id-column-launches
branch
from
ff32a99
to
d442cb8
Compare
marcospri
force-pushed
the
ext_lti_assignment_id-column-frontend
branch
from
196834c
to
d46b786
Compare
marcospri
force-pushed
the
ext_lti_assignment_id-column-launches
branch
from
d442cb8
to
cba0950
Compare
marcospri
changed the base branch from
ext_lti_assignment_id-column-frontend
to
ext_lti_assignment_id-column-view
marcospri
force-pushed
the
ext_lti_assignment_id-column-launches
branch
from
0752227
to
667b97b
Compare
marcospri
force-pushed
the
ext_lti_assignment_id-column-view
branch
3 times, most recently
from
932e114
to
2d38f0f
Compare
marcospri
force-pushed
the
ext_lti_assignment_id-column-launches
branch
from
667b97b
to
a0612b0
Compare
marcospri
commented
Sep 29, 2021
marcospri
commented
Sep 29, 2021
marcospri
force-pushed
the
ext_lti_assignment_id-column-launches
branch
from
a0612b0
to
ad3e4c1
Compare
marcospri
force-pushed
the
ext_lti_assignment_id-column-view
branch
from
2d38f0f
to
e6dc269
Compare
marcospri
force-pushed
the
ext_lti_assignment_id-column-launches
branch
from
ad3e4c1
to
aa07875
Compare
seanh
requested changes
Sep 30, 2021
marcospri
force-pushed
the
ext_lti_assignment_id-column-view
branch
6 times, most recently
from
c97d799
to
90b224e
Compare
marcospri
force-pushed
the
ext_lti_assignment_id-column-launches
branch
3 times, most recently
from
e73b33e
to
70794ef
Compare
marcospri
commented
Oct 18, 2021
| tool_consumer_instance_guid = request.params.get("tool_consumer_instance_guid") | ||
|
|
||
| return ( | ||
| assignment_svc.exists(tool_consumer_instance_guid, resource_link_id) | ||
| assignment_svc.exists( |
There was a problem hiding this comment.
This will find assignments created by the new API (once the FE part is merged) but not yet launched
| name = "canvas_file" | ||
|
|
||
| def __call__(self, context, request): | ||
| return ("canvas_file" in request.params) == self.value | ||
| return ("canvas_file" in request.params) == self.value and self.db_configured( |
There was a problem hiding this comment.
Exclude db_configured ones
| def __call__(self, context, request): | ||
| return ("url" in request.params) == self.value | ||
| return ("url" in request.params) == self.value and self.db_configured( |
There was a problem hiding this comment.
Exclude db_configured ones
seanh
approved these changes
Oct 18, 2021
| @@ -93,16 +93,18 @@ def store_lti_data(self): | |||
| @view_config(canvas_file=True) | |||
| def canvas_file_basic_lti_launch(self): | |||
| """ | |||
| Respond to a Canvas file assignment launch. | |||
| Respond to a Canvas file assignment launch which is not db_configured. | |||
There was a problem hiding this comment.
Should we rename this view to legacy_canvas_file_basic_lti_launch()?
lms/views/basic_lti_launch.py
Outdated
| # | ||
| # We need to merge the two assignments into one. | ||
|
|
||
| # order is guaranteed by the query's `order by` |
There was a problem hiding this comment.
Suggested change
| # order is guaranteed by the query's `order by` |
I think we should remove the comment as it's unclear that the query refers to (since it refers to a SQLAlchemy query in another file). It's part of get_for_canvas_launch()'s contract/docstring that if it returns two assignments it'll be the old one first:
lms/lms/services/assignment.py
Line 74 in a6b7e55
marcospri
force-pushed
the
ext_lti_assignment_id-column-launches
branch
from
70794ef
to
c0cdab2
Compare
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.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Without #3126 this PR should still show not behavior changes.
The new
canvas_db_configured_basic_lti_launchwill handle newly configured assigments, existing ones and the ones that need merging.Note that in this PR (pending the merging of the fronted part) canvas assignment are both url and db configured, that's why we needed to explicitly exclude db_configured ones from the url_conifgured predicate.
The rationale here is to merge this, start storing all assignment in the DB and later stop sending canvas the URL, file id, group set... and have a legacy_canvas_assignment (with params from the URL) and switch the
canvas_db_configured_basic_lti_launchto pick the params from the DB.