DN-3840: Show photos from Canvas - #217
Conversation
goomens
left a comment
goomens
left a comment
There was a problem hiding this comment.
Question: is there a way to get picture urls from Canvas without the user logging in via Canvas? If we would want to do some bulk operation via the API?
|
|
||
| [BsonElement("Picture")] | ||
| [BsonIgnoreIfNull] | ||
| public string? Picture { get; set; } |
There was a problem hiding this comment.
This is an url I think? Should we call it PictureUrl?
There was a problem hiding this comment.
Yes it's the url of the user avatar. I used Picture to match LtiPrincipal of the canvas response. But I agree that maybe PictureUrl would be clearer.
| } | ||
| } | ||
|
|
||
| // Enrich Picture on embedded instance user snapshots from the live users table |
There was a problem hiding this comment.
Is this a logical place to do this? Do we always want to do this? Most of the time a context won't need users or pictures I guess
There was a problem hiding this comment.
Yeah I don't really know what a good place would be. Now it runs every time an instance is opened so you have the updated pictures. Do you have another suggestion?
This currently uses the |
|
I'm now syncing all the info for users to the instances. Because I guess we don't want stale data in the instances. But that also means the name of the person updates to the name in Canvas. Do we want that as the source of truth? Or only for pictures? |
…tically by mongo
It should always be the same but maybe best not to rely on Canvas because not all users come from Canvas. So let's use an another single source of truth for that |
So do we want to sync the email address or the organisation? |
|
No, let's not use Canvas for any of that |
| /// Finds all instances containing this user in any user-type property and replaces the | ||
| /// fields in the embedded snapshot with the value from the current User state. | ||
| /// </summary> | ||
| public async Task SyncUserInInstances(User user, string[] fields, CancellationToken ct) |
There was a problem hiding this comment.
For strong typing, do we maybe want to use expressions here instead of strings?
There was a problem hiding this comment.
I've made some changes to fix this
| foreach (var instance in instances) | ||
| { | ||
| if (SyncUserInInstance(instance, user, fields)) | ||
| await instanceRepository.Update(instance, ct); |
There was a problem hiding this comment.
I think we want to avoid using this whenever possible because it overwrites the full instance, which can cause very strange effects when operations are performed simultaneously. Can we use a property-level update here?
There was a problem hiding this comment.
I've made some changes
…the instance instead of the whole
2 participants
uses this new version of the LTI-consumer package uva/LTI-consumer#8