I think you may have missed adding some files - I don't see the MattermostIntegration code in here, for example. Are you sure you used git add before committing?

Most of the stuff in your new "basechat" implementation is pretty Slack specific. This works because Mattermost tries to duplicate Slack's formatting very carefully, but that won't generalize to more services. I suspect what we want is to have the basechat integration set up all the handlers and create some kind of intermediate data structures, which the individual subclasses can then format into whatever is necessary. Because Mattermost borrows a lot from Slack, we could have utility code for that in the slack/ module that Mattermost imports.

All the files inside .idea shouldn't be part of your commit.

We also need an icon@2x.png file.

You've still got this .idea file in the change.

We should include module docstrings at the top of each new file (something like """Forms for chat integrations""")

I don't think it was before, but this string should be wrapped in _()

I don't think it was before, but this string should be wrapped in _()

Module docstring.

Shouldn't mention slack here.

Shouldn't mention slack.

These could all go on the same line.

Shouldn't mention slack here.

Shouldn't mention slack here.

Hmm. This is probably something that we should have an abstract method for in basechat and specialize per subclass.

Shouldn't mention slack here.

Shouldn't mention slack here.

Shouldn't mention slack here.

Logging here shouldn't mention slack.

Shouldn't mention slack here.

Shouldn't mention slack here.

Shouldn't mention slack here.

Shouldn't mention slack here.

We can just skip this entirely because the base class for all integrations defines it.

Module docstring.

Too many blank lines here.

Not too many blank lines enough here.

Because this is a property, we can document it like a property (just a single line `"""The icons used for the integration.""")

Module docstring

Class docstring

Don't need this blank line.

Should include a docstring.

Looks like we can just get rid of this file, no?

Maybe mention that this is oriented towards Slack, but broken out of the integration class because other services (like Mattermost) duplicate Slack APIs.

This argument should be called integration instead of self

Perhaps just "configured channels"? I think it's definitely possible that other slack clones will pop up in the future.

I think an argument needs a default value in order for it to be optional, I'm not seeing a default value for pre_text or fallback_text in the method declaration.

If fallback_text is optional, I'm guessing that means it can be empty. If it is empty, the new value of fallback_text would have an extra : in it. Might be worth adding extra logic for checking if fallback_text is empty and removing the extra :.

Same thing here with a possibly empty fallback_text getting passed in.

This import is coming from the same location as the line above, this can be updated to a single line with from rbintegrations.slack.integration import utility_format_link, utility_notify.

There might be a good reason why this line was moved away from the django imports, but having the line here doesn't follow the standard of having third party imports before local imports.

Ship It!