Adding the remoteID migration, and making the synthetic user identification more robust

[jespino]

This should migrate the remoteID from the old users into the plugin remoteID, also ensure that there is always a remote ID, by default is msteams, but as soon as you register the remote on the shared channels it gets updated to a valid one. The reason to have a default remoteID is because not always we call the SharedChannels remote register. Probably we should do it always, and we will do eventually.

The migration is executed in every plugin restart, but in theory it is idenpotent, so it shouldn't be a problem.

[jespino]

Can you confirm if this check is needed? I would like to always register the SharedChannels for the plugin.

[jespino]

@wiggin77 ☝️

[wiggin77]

I think we want an easy way to to turn this off in case of a problem in production. However we don't want to use a fake remoteID either since that may be hard to clean up later. I would suggest calling RegisterPluginForSharedChannels always, but then calling UnregisterPluginForSharedChannels if p.getConfiguration().DisableSyncMsg == true. That way you will have the real remoteID.

[jespino]

My concern is the remoteID changing over disabling/enabling. Is the remote id going to be stable or it is going to change every time that I unregister the plugin?

[jespino]

Anyway, I applied the changes that you proposed.

[lieut-data]

Either way we want the same remoteID for every unique plugin ID.

This is probably fine for our current purposes, but thinking (very) long term, I could also see us supporting multiple Microsoft tenants concurrently which might necessitate a distinct remoteID for each (if we wanted to, for example, distinguish each in the UI?)

Stepping back, I'm also just wanting to confirm that /we/ (the plugin) don't really care about the value here at all, and that it's just an artifact of the way the shared channels subsystem works with users that happen to have a RemoteID set. In other words, we want all DMs and all GMs for all users, regardless of whether anyone has a RemoteID set. Does that remain accurate @wiggin77?

[wiggin77]

If you set the appropriate flag when registering then yes, the MS Teams plugin will get sync for all DM/GMs regardless of remoteID value. Other plugins using the API will likely not set that flag and the remoteID on posts/users/etc will matter.

Should we modify the register API to accept a remoteID, thus allowing a plugin to call register for each tenant? That would also leave it up to the plugin to ensure idempotency. cc @jespino @lieut-data

[lieut-data]

Should we modify the register API to accept a remoteID, thus allowing a plugin to call register for each tenant? That would also leave it up to the plugin to ensure idempotency. cc @jespino @lieut-data

On the one hand, I don't want to preemptively build for the future. But I do like the idea of giving the plugin control over the "primary key" backing the cursor based model. At the moment, it feels just a bit too decoupled. So I'm in favour of the idea. Is the RemoteId constrained by some length we should remain aware of? (i.e. is it long enough for a UUID?)

[wiggin77]

It's the same length as most IDs in MM.

[lieut-data]

Circling back on this from a discussion with @jespino: I'm agreeing with his proposal to just use the RemoteID as-is and deal with the multi-tenant situation in the future (if needed). This unblocks things and gets us closer to being able to rely on shared channels for message delivery. So essentially no changes required on this thread.

[wiggin77]

Looks great. I think we need the real remoteID which we can get by always registering then selectively unregistering based on config.

[lieut-data]

Thanks, @jespino! There's one blocker below, but it's in the unit tests, otherwise just a handful of non-blocking questions so I'm approving to unblock.

[jespino]

@lieut-data applied the changes, and I hope I also answered your doubt.

[wiggin77]

LGTM 👍