WIP Canvas assignments in the DB #3115

marcospri · 2021-09-16T06:42:54Z

No description provided.

marcospri

To edit the assignments I'm using ext_lti_assignment_id, stored in extra in this PR.

The main question here is: do we keep both hypothesis_assignment_id and use ext_lti_assignment_id too avoid duplicates while editing? Or just use ext_lti_assignment_id?

There's different possibilities when using ext_lti_assignment_id, we could for example keep the column, but give it the value from ext_lti_assignment_id instead of generating one ourselves. We could just store it in extra without adding new columns.

Or we could use both ids independently as I'm doing in this PR but that's a bit of an accident.

I think I'm leaning towards:

adding a new column, generically named hypothesis_id or similar
Make it unique with tool_consumer_guid (as is not very clear if it's unique globally, probably it is as it looks as UUID, but just in case https://community.canvaslms.com/t5/Canvas-Question-Forum/Is-ext-lti-assignment-id-field-globally-unique/td-p/402981)
Make the constraint for both resource_link_id and the new column not being null at the same time.
Explicitly pass the hypothesis_id to assignment_service.create instead of generating one inside.

I think that way we could have the explicit column but not have a separate concept/id for the updates.

lms/models/assignment.py

lms/views/assignment.py

marcospri · 2021-09-16T06:50:43Z

lms/static/scripts/frontend_apps/components/FilePickerApp.js

+      async function fetchHypothesisId() {
+        const data = {extra: createAssignmentAPI.data, content: content };
+        const assignment = await apiCall({
+          authToken,
+          path: createAssignmentAPI.path,
+          data: data,
+        });
+        console.log("GOT ID")
+        setHypothesisId(assignment['hypothesis_assignment_id']);
+      }
+
+      if (content && createAssignmentAPI && !hypothesisId) {
+          fetchHypothesisId();
+          return
+      }
+


This is probably all madness but wanted to test the full flow.

marcospri · 2021-09-16T06:52:06Z

lms/views/assignment.py

+        print(self.request.json_body)
+        assignment = self.assignment_service.create(
+            self.application_instance.tool_consumer_instance_guid,
+            self.request.json_body["content"]["url"],


Only works for URL assignments. It needs a bit of work to map other type of assignment (canvas files) to a document_url here.

marcospri · 2021-09-16T07:46:38Z

lms/static/scripts/frontend_apps/components/FilePickerFormFields.js

 }) {
  /** @type {Record<string,string>} */
  const extraParams = groupSet ? { group_set: groupSet } : {};
+  if (hypothesisId) {


Should we not send the url of the document to canvas anymore? I think we still should send it even if we don't use it.

lms/services/assignment.py

lms/models/assignment.py

lms/services/assignment.py

marcospri · 2021-09-16T12:57:53Z

lms/static/scripts/frontend_apps/components/FilePickerFormFields.js

 }) {
  /** @type {Record<string,string>} */
  const extraParams = groupSet ? { group_set: groupSet } : {};
+  if (extLTIAssignmentId) {
+    extraParams['ext_lti_assignment_id'] = extLTIAssignmentId;


This is the name of the param we submit to canvas. Should we rename to both something shorter and namespaced with hypo_ or similar?

I think I'm leaning towards just using the LTI name. Why replace it with our own?

I think I'm leaning towards just using the LTI name. Why replace it with our own?

I was just thinking on the length in the URL, that's 36 chars for the value and 21 for the parameter name

marcospri · 2021-09-16T12:59:30Z

lms/views/predicates/_lti_launch.py

@@ -33,14 +33,14 @@ def db_configured_assignment_launch_view(context, request):
    def __call__(self, context, request):
        assignment_svc = request.find_service(name="assignment")
        resource_link_id = request.params.get("resource_link_id")
+        ext_lti_assignment_id = request.params.get("ext_lti_assignment_id")


Changes in this file make the changes backwards compatible. Existing url configured ones will still go thru that code path, new ones will fall into the DB configured ones.

Needs changes on canvas file ones.

marcospri · 2021-09-16T13:01:11Z

lms/services/assignment.py

+                    (Assignment.resource_link_id == resource_link_id)
+                    | (Assignment.ext_lti_assignment_id == ext_lti_assignment_id)


Not sure if clear from the comment but this can't be an and there will be a time when we have stored the ext_lti_assignment_id but not the resource_link_id but we have both !=None in this function (on first launch).

And I guess we have pre-existing assignments that have a resource_link_id but no ext_lti_assignment_id. And also non-Canvas assignments too. So we do need to support looking up by resource_link_id only.

I'm wondering about this query and None / NULL though. For example if the call is get(tool_consumer_instance_guid, resource_link_id, ext_lti_assignment_id=None) won't that match all rows that have NULL in the ext_lti_assignment_id?

lms/migrations/versions/64c815401627_assigment_new_id.py

seanh · 2021-09-16T13:19:59Z

lms/migrations/versions/64c815401627_assigment_new_id.py

+        "module_item_configurations",
+        "resource_link_id",
+        existing_type=sa.VARCHAR(),
+        nullable=True,


Do you need the existing_type here? I think you can just do op.alter_column("module_item_configurations", "resource_link_id", nullable=True).

lms/migrations/versions/64c815401627_assigment_new_id.py

lms/models/assignment.py

seanh · 2021-09-16T14:53:12Z

lms/resources/_js_config/__init__.py

@@ -212,7 +224,7 @@ def enable_content_item_selection_mode(self, form_action, form_fields):
                "filePicker": {
                    "formAction": form_action,
                    "formFields": form_fields,
-                    # Specific config for pickers
+                    "createAssignmentAPI": self._create_assignment_api(),


lms/routes.py

seanh · 2021-09-16T15:33:53Z

lms/services/assignment.py

+                    (Assignment.resource_link_id == resource_link_id)
+                    | (Assignment.ext_lti_assignment_id == ext_lti_assignment_id)


And I guess we have pre-existing assignments that have a resource_link_id but no ext_lti_assignment_id. And also non-Canvas assignments too. So we do need to support looking up by resource_link_id only.

I'm wondering about this query and None / NULL though. For example if the call is get(tool_consumer_instance_guid, resource_link_id, ext_lti_assignment_id=None) won't that match all rows that have NULL in the ext_lti_assignment_id?

seanh · 2021-09-16T15:34:59Z

lms/services/assignment.py

+        if resource_link_id and not assignment.resource_link_id:
+            # First lunch of the assignment, fill the resource_link_id now that we have it.
+            assignment.resource_link_id = resource_link_id


What about the case where we find an assignment with a matching ext_lti_assignment_id and it has a different resource_link_id than the one we received in the launch params? That should never happen, but maybe we want to make sure that we catch that case and raise an error

seanh · 2021-09-16T15:37:04Z

lms/static/scripts/frontend_apps/components/FilePickerFormFields.js

 }) {
  /** @type {Record<string,string>} */
  const extraParams = groupSet ? { group_set: groupSet } : {};
+  if (extLTIAssignmentId) {
+    extraParams['ext_lti_assignment_id'] = extLTIAssignmentId;


I think I'm leaning towards just using the LTI name. Why replace it with our own?

marcospri · 2021-09-21T13:04:12Z

More concrete PRs created after this. Summary on #3127

marcospri force-pushed the hypothesis-assignment-id-column branch from 0cdfa55 to 68ba08e Compare September 16, 2021 06:52

marcospri commented Sep 16, 2021

View reviewed changes

lms/services/assignment.py Outdated Show resolved Hide resolved

marcospri force-pushed the hypothesis-assignment-id-column branch from 68ba08e to fb34a7f Compare September 16, 2021 12:45

WIP

0190d6a

marcospri force-pushed the hypothesis-assignment-id-column branch from fb34a7f to 0190d6a Compare September 16, 2021 12:56

marcospri commented Sep 16, 2021

View reviewed changes

marcospri requested a review from seanh September 16, 2021 13:01

seanh approved these changes Sep 16, 2021

View reviewed changes

f

db2730c

marcospri closed this Sep 21, 2021

marcospri deleted the hypothesis-assignment-id-column branch November 10, 2021 16:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP Canvas assignments in the DB #3115

WIP Canvas assignments in the DB #3115

marcospri commented Sep 16, 2021

marcospri left a comment •

edited

Loading

marcospri Sep 16, 2021

marcospri Sep 16, 2021

marcospri Sep 16, 2021

marcospri Sep 16, 2021

seanh Sep 16, 2021 •

edited by marcospri

Loading

marcospri Sep 17, 2021

marcospri Sep 16, 2021

marcospri Sep 16, 2021

seanh Sep 16, 2021

seanh Sep 16, 2021

seanh Sep 16, 2021

seanh Sep 16, 2021

seanh Sep 16, 2021

seanh Sep 16, 2021

seanh Sep 16, 2021 •

edited by marcospri

Loading

marcospri commented Sep 21, 2021

		(Assignment.resource_link_id == resource_link_id)
		\| (Assignment.ext_lti_assignment_id == ext_lti_assignment_id)

WIP Canvas assignments in the DB #3115

WIP Canvas assignments in the DB #3115

Conversation

marcospri commented Sep 16, 2021

marcospri left a comment • edited Loading

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

seanh Sep 16, 2021 • edited by marcospri Loading

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

Choose a reason for hiding this comment

seanh Sep 16, 2021 • edited by marcospri Loading

Choose a reason for hiding this comment

marcospri commented Sep 21, 2021

marcospri left a comment •

edited

Loading

seanh Sep 16, 2021 •

edited by marcospri

Loading

seanh Sep 16, 2021 •

edited by marcospri

Loading