search

mattermost / mattermost-plugin-msteams

Other
12 stars 10 forks source link

rm -fr Teams Primary preference #673

Closed lieut-data closed 1 month ago

lieut-data commented 1 month ago

Summary

Rip out all things re: Teams Primary & automute semantics. Our test coverage made me feel much more comfortable doing this, with smoke testing confirming general functionality remaining intact.

Ticket Link

Fixes: https://mattermost.atlassian.net/browse/MM-58439

lieut-data commented 1 month ago

Don't we still need this when SelectiveSync = true and SyncRemoteOnly = false. I thought we wanted to keep this functionality, which is why we added SyncRemoteOnly as a config setting.

We met with the one customer using this feature and agreed to deprecate this functionality and shift towards a model where everyone is Mattermost primary. (Apologies for this, @sbishel, as this took extra time to resolve and we could have otherwise avoided your extra work.)

Selective sync as a concept still makes sense without this feature, but I suspect we can simplify SyncRemoteOnly and SelectiveSync further to remove even more code.

lieut-data commented 1 month ago

I don't think SyncRemoteOnly is even necessary any longer.

@sbishel, let's double check with @esethna, but maybe we rebrand SelectiveSync as what SyncRemoteOnly means (eliminationg the flag, but not the functionality)? I'm unsure though :)

sbishel commented 1 month ago

@sbishel, let's double check with @esethna, but maybe we rebrand SelectiveSync as what SyncRemoteOnly means (eliminationg the flag, but not the functionality)? I'm unsure though :)

Yeah...that's what I meant... SelectiveSync == SyncRemoteOnly.

lieut-data commented 1 month ago

making a cleanup migration to remove the preferences set in the DB. Would be nice if the plugin was in production but probably not a big issue right now.

@JulienTant, I did this manually on msteams-sync-test, and will probably suggest doing the same for customers migrating to this version. In principle, we /could/ add a migration to achieve this, but given we're still pre-production, I'm inclined to avoid the extra churn. Thoughts?

JulienTant commented 1 month ago

@lieut-data

I'm inclined to avoid the extra churn. Thoughts?

I agree that's why I was 0/5; I thought I'd mention it just in case it's something you wanted to add but I also don't think it's necessary right now.

esethna commented 1 month ago

maybe we rebrand SelectiveSync as what SyncRemoteOnly means

Yeah...that's what I meant... SelectiveSync == SyncRemoteOnly.

+1 to this.