search

blindsidenetworks / mattermost-plugin-bigbluebutton

BigBlueButton plugin for Mattermost :electric_plug:
Apache License 2.0
89 stars 43 forks source link

Full cleanup #59

Closed ghost closed 4 years ago

ghost commented 4 years ago

@ffdixon I've fixed the majority of the issues reported in full code review. For bigger changes, I've filed some issues which can be addressed later. This also allows us to prioritize the search reporting query optimization changes.

cadavre commented 4 years ago

Just made a quick review of PR. Looks to fix few of issues I was about to create.

Looking forward to get it merged!

Good job @harshilsharma63 !

cadavre commented 4 years ago

Please keep in mind that rename salt -> secret will likely broke backward compatibility because of plugin config stored with salt key.

Best would be to provide migration or fallback.

ffdixon commented 4 years ago

A fallback makes sense so nothing current breaks.

cadavre commented 4 years ago

I'd suggest to release 2.1.0 with with being merged.

ghost commented 4 years ago

Please keep in mind that rename salt -> secret will likely broke backward compatibility because of plugin config stored with salt key.

Best would be to provide migration or fallback.

Good catch! We can keep the setting key as original and just update the variable and display names.

ghost commented 4 years ago

Tested this on latest Mattermost v5.18, everything works great! Proceeding with merging.

ghost commented 4 years ago

@ffdixon can you merge this PR for me? I don't have permission to merge to master.