mattermost / mattermost-plugin-mscalendar

Mattermost plugin for Microsoft Office365
Apache License 2.0
13 stars 20 forks source link

Followups for shared calendar engine PR #267 #357

Open mickmister opened 4 months ago

mickmister commented 4 months ago

As a follow up to https://github.com/mattermost/mattermost-plugin-mscalendar/pull/267, I've documented some left over cleanup items that mostly pre-existed in the codebase before that calendar migration PR.

  1. Consider removing unused commands

There are a few commands that, to my knowledge, are never used. Either because they are outside of the purpose of the plugin, or they require meticulous datetime formatting in the command arguments. Here are some that I noticed:

    case "createcal":
    case "createevent":
    case "deletecal":
    case "findmeetings":
    case "showcals":
    case "events":
    case "avail":
    case "subscribe":
    case "unsubscribe":

We'll want to delete these from the processed commands, and command autocomplete.

  1. In the EventResponder struct, there are 4 methods. One of the methods RespondToEvent essentially implements the other three, and those other three methods are never called and so are dead code. The methods not being called should be removed.

https://github.com/mattermost/mattermost-plugin-mscalendar/blob/1de44d0a7bc039b95f3877ba6a560845e3b4ab75/calendar/engine/event_responder.go#L10-L15

  1. Document how the status sync job works. It's currently not obvious how the timing window stuff works at the code block below. It should be clear how big the window is and how close by to the event we want to calculate certain things.

https://github.com/mattermost/mattermost-plugin-mscalendar/blob/1de44d0a7bc039b95f3877ba6a560845e3b4ab75/calendar/engine/availability.go#L21-L29

  1. Logging in the renew job is excessive - We create a log message for every connected user whenever the renew job runs. We should consider the value in this. Maybe we should only log on error logging during renew job seems overkill

https://github.com/mattermost/mattermost-plugin-mscalendar/blob/1de44d0a7bc039b95f3877ba6a560845e3b4ab75/calendar/jobs/renew_job.go#L31-L34

  1. There are references to mscalendar in the plugin setup. It should be made more agnostic like pluginBot instead of mscalendarBot

https://github.com/mattermost/mattermost-plugin-mscalendar/blob/1de44d0a7bc039b95f3877ba6a560845e3b4ab75/calendar/plugin/plugin.go#L175