litaio / lita

ChatOps for Ruby.
https://www.lita.io
MIT License
1.68k stars 179 forks source link

Ability to store metadata for a user? #63

Open esigler opened 10 years ago

esigler commented 10 years ago

Howdy! I know the window is closing for Lita 4.0, so apologies in advance if this is possible and I just haven't seen it yet - Would it be possible / reasonable to be able to store metadata about a user that is cross-plugin accessible?

For a concrete example, @theckman's Github plugin will soon be able to store a per-user OAuth token. I have a different not-yet-released plugin that will also need a users's Github OAuth token to take certain actions. It would be nice to be able to share that information without having to make a specific agreement between the two plugins.

It kind of seems like this would be possible by re-creating the user with new metadata, like in the current spec, but is that recommended?

jimmycuadra commented 10 years ago

Users are designed to have arbitrary metadata stored with them: https://github.com/jimmycuadra/lita/blob/c770cd80634311fa990df57524ab9ea49dad9e74/lib/lita/user.rb#L110 The example you linked to is indeed an encouraged usage. Even the user's name and mention name are just metadata – the only user information that is not considered metadata is their ID. I'll leave this issue open to remind me to make this more apparent in the docs.

theckman commented 10 years ago

So I've been thinking about this. The API for accomplishing this is not what I would expect. For user objects I would expect there to be a CRUD (create, read, update, delete) interface. It seems like the create() method is a bit more intelligent than I'd expect it to be. I would expect that to fail if the user already exists instead of trying to merge attributes. Was there a specific technical limitation that prompted doing it this way?

I like the idea of something like this, but I may be the minority:

u = Lita::User.new(42)
u.update(github_token: 'abc123')
u.metadata
# imagine the metadata being printed here including the github_token :p

I'm betting there is something here I am not thinking about. Was there some limitation for using the current API for the users?

jimmycuadra commented 10 years ago

It's a bit of an odd history. create and find were originally aliases of the same method which, as it still does, behaved more as a find_or_create. Since users are generally created in response to receiving a message from them for the first time, a single method for "get a persisted Lita::User object for the user with this information" seemed ideal. However, I take your point that the conflation of behavior may not be very intuitive, especially since the method is just called create now.

Of course, it'd be possible to separate the different operations into more standard CRUD methods in a future version of Lita, though I'm hesitant to break an API unless forced by a use case. If there are things people want to do that are not possible with the current user API, that would be the best way to influence the design of a new API.

theckman commented 10 years ago

That explains the history. I can understand the hesitation on breaking the API. Something like this would be painful as it's something integral that the adapters use. It definitely needs to be a bigger improvement than just usability. I guess the best we can do now is make the documentation on manipulating the user metadata a bit more explicit.

jimmycuadra commented 10 years ago

I think the only thing that's actually missing, in terms of functionality, is the ability to delete a user after it's been created. Everything else can be done with the find_by_* methods, metadata, and save. The fact that create will simply merge with an existing user with the same ID can largely be ignored. I'll add a new section to the docs site that covers managing users.