Mismatches on build_identifier()

[JunaidLoonat]

From errbot documentation, send() requires a valid identifier as created by the build_identifier().

Unfortunately, the existing mattermost-backend build_identifier() does not play nice with a single string identifier as used in the example. Moreover, the existing build_identifier() does not return a single identifier object (as done so within the other backends).

Lastly, there's no documentation on what string identifiers may be used by the mattermost-backend's build_identifier() in order to create valid identifier objects.

[Vaelor]

It should be an easy fix, I just had a look at this and I hope/think I can fix that before my holiday.

I did not think about it when I build the build_identifier(), so thanks for pointing it out! Mattermost returns an array of userIds like that: 'mentions': '["xk37oq13k7b8dgh8ttbyityfrh"]' Since I oriented myself at 'what mattermost does' currently you would need to pass the userid to build_identifier() (which is of course totally useless like that :-D).

I guess my build_identifier() is wrong when it returns an Array of mentions (being called build_identifier, not build_identifiers()) - because the callback_mention() seems to be able to notify multiple users, I at that time thought that this would be correct.

So my goal would be:

• build_identifier() should take a username, a simple string like 'name' or '@name'
  • Unsure how to deal with the room at that point, maybe the room part would come after my holiday. But when it does, it would be something like ~room, like Mattermost does.
• build_identifier() should return a MattermostPerson, MattermostRoom or MattermostRoomOccupant
• Add a documentation somewhere

Thanks!

[Vaelor]

Oh I just saw that you made a pull request! Sorry, I just looked at the Issue, not seeing your PR!!

[Vaelor]

Ok, I merged your PR, but I would like to keep that issue open to remind me to have a look at the MattermostRoomOccupant (the Slack backend does include a SlackRoomOccupant, so it would probably make sense to do the same)

[JunaidLoonat]

Originally, I actually wrote code to return MattermostRoomOccupant as well. However, I didn't see any major difference in the behavior between returning that or a MattermostPerson, so I dropped it. I figured it would be better for me to submit a simple PR that improves on existing code, rather than try to fix everything (potentially incorrectly) in one commit.

I agree that we keep this open for a bit.