Sort sectionNames var expansion by section id

[carl-codeorg]

Disclaimer: This is my first attempt at a PR against the Canvas repo; I'm a developer at Code.org, which is part of the partner program. I've been told that I should be covered under the partner agreement and don't need to submit a signed contributor agreement. Please let me know if my PR is lacking anything as it's my first try!

PR context: The variable expansion for com.instructure.User.sectionNames has no explicit order. While this is fine when the variable is used in isolation, it becomes problematic when an LTI tool needs to construct full knowledge of a course's sections using a combination of com.instructure.User.sectionNames and Canvas.course.sectionIds. Since the IDs come back in ascending order, it makes sense to sort the names by the same property, guaranteeing that names can be mapped accurately onto IDs, and enabling the tool to accurately reconstruct the sections for a given course using only the NRPS membership response.

For reference, here is the code that returns sorted sectionIds in the substitutions_helper:

def section_ids
  course_enrollments.map(&:course_section_id).uniq.sort.join(",")
end

This should reliably sort the sectionIds in ascending order. However, looking at the variable expansion code for com.instructure.User.sectionNames, we see that there is no order clause:

@context.enrollments.active.joins(:course_section).where(user_id: @current_user.id).pluck(:name)&.to_json

This seems to return the sectionNames for a user in the order in which the user was enrolled in each section. In practice this makes it possible to know which sections a user is in (since we have the IDs) and the names of the sections (we have all the names), but not which sections map to which names, because the order of the names is not guaranteed. This is also coming from the perspective of a third-party tool integrating with Canvas via LTI 1.3, using the NRPS API, but not the more full-featured Canvas REST APIs.

This PR simply orders the names by section id. Since no order was guaranteed or documented before, I'm hoping this wouldn't be considered a breaking change for users, and I think it solves a big use case for LTI tool providers.

[CLAassistant]

All committers have signed the CLA.

[alivira]

FYI for more context - We're from Code.org and are developing LTI integrations to Canvas to enable single sign-on and roster sync for our network of teachers & students (~80m users all-up). We're hoping to launch this integration this year - but noticed that we can't tie Section IDs to Section Names since the NPRS call returns IDs and Names in a separate order. This PR should improve that behaviour & enable other integration partners to light up on Canvas as well. Happy to provide more info or hop on a call if needed - please let us know!

[slaughter550]

This looks good. We're going to bring it into our CI pipeline and let you know the results. It might take a bit but I'll try to help move this along. Thanks for the contribution!

[alivira]

Thanks @slaughter550! Do you happen to know how long the CI deploy for this kind of fix typically takes?

[bdmesh]

@slaughter550 - I work with Carl and Ali. Thank you so much for the help! I'm hoping you might be able to comment on the ETA for this. We're coming up on a critical point in our delivery and are dependent upon this change.

Our only alternative workaround is significant and would set my team back weeks to implement. So we're really hoping that this change might find it's way into a deployment within the next week. You'd be doing us such a huge solid if you could help get the change through that final step or even just sharing an ETA.

Thanks!

[xandroxygen]

Sorry for the delay on this! We pulled it into our CI system, and merged it from there: https://github.com/instructure/canvas-lms/commit/bc138df716f09ae27d775099127c943a7ed6ef17

I will work on getting this deployed. Thank you for submitting the PR!!

[carl-codeorg]

That's awesome @xandroxygen, thanks for merging it in! Do you know roughly how long it might be before the change makes its way into production?

[xandroxygen]

Since we kind of dropped the ball on getting this merged in a reasonable time frame, I am going to hotfix it today

[xandroxygen]

@carl-codeorg it's deployed, lmk if you run into any troubles. I will have this PR closed 👍

[carl-codeorg]

@xandroxygen I've just tested and everything looks great from my end; the issue we were experiencing is fixed for my test course!

Thanks for the quick deployment. Looking forward to getting our Canvas integrations fully up and running now that we're unblocked!

[bdmesh]

@xandroxygen and @slaughter550 - thank you thank you! This is a huge for us. Thanks a ton for getting this pushed through!!