skoczen / will

Will is a simple, beautiful-to-code bot for slack, hipchat, and a whole lot more.
https://heywill.io
MIT License
407 stars 170 forks source link

Get current room's history #89

Closed ghost closed 9 years ago

ghost commented 9 years ago

It would be awesome, if Will could read the current room's history. I'm writing a plugin and it needs this functionality. It's not much code, don't get me wrong, but it relies on the "requests" library, and I really hate 3rd party libraries in plugins. Here's how it's done with requests:

def get_history(self, room_id):
    payload = {"auth_token": self.auth_token}
    resp = requests.get("https://xxxxx.hipchat.com/v2/room/{0}/history".format(str(room_id)),
                        params=payload)
    return resp.text

I hope that this snippet can help you a little bit and that you can implement this somehow in the near future.

skoczen commented 9 years ago

FWIW, will already relies on requests. :)

https://github.com/skoczen/will/blob/master/requirements.txt

I'd say the odds of me implementing it in the near future are honestly pretty slim - I don't need that functionality, and so it's going to be prioritized behind work-work, and other OSS projects that are blocking me.

However, if you're game for implementing it, I'd be happy to talk about where this would fit into the API, and then open up the implementation to you!

ghost commented 9 years ago

I didn't notice until after posting this, that will already relies on requests.

I'd be glad to implement this feature myself. I didn't yet dive into will's core but I'm sure that it shouldn't be the hardest of challenges.

skoczen commented 9 years ago

Fantastic - give me a few minutes, and I'll whip up some pseudocode on where to put it in core!

skoczen commented 9 years ago

Alright, here's a really rough skeleton, but with all the bits in places that at least on first glance, make good sense to me. Let me know if anything seems awry.

in room.py

import requests

from will import settings
from will.utils import Bunch

V1_TOKEN_URL = "https://%(server)s/v1/rooms/list?auth_token=%(token)s"
V2_TOKEN_URL = "https://%(server)s/v2/room?auth_token=%(token)s"

class Room(Bunch):

    @property
    def history(self):
        # history = requests() # make api request
        # return history
        return "Foo"

class RoomMixin(object):
    def update_available_rooms(self, q=None):
        self._available_rooms = {}
        # Use v1 token to grab a full room list if we can (good to avoid rate limiting)
        if hasattr(settings, "V1_TOKEN"):
            url = V1_TOKEN_URL % {"server": settings.HIPCHAT_SERVER,
                                  "token": settings.V1_TOKEN}
            r = requests.get(url)
            if r.status_code == requests.codes.unauthorized:
                raise Exception("V1_TOKEN authentication failed with HipChat")
            for room in r.json()["rooms"]:
                self._available_rooms[room["name"]] = room
        # Otherwise, grab 'em one-by-one via the v2 api.
        else:
            url = V2_TOKEN_URL % {"server": settings.HIPCHAT_SERVER,
                                  "token": settings.V2_TOKEN}
            resp = requests.get(url)
            if resp.status_code == requests.codes.unauthorized:
                raise Exception("V2_TOKEN authentication failed with HipChat")
            rooms = resp.json()

            for room in rooms["items"]:
                url = room["links"]["self"] + "/?auth_token=%s;expand=xmpp_jid" % (settings.V2_TOKEN,)
                room_details = requests.get(url).json()
                # map missing hipchat API v1 data
                for k, v in room_details.items():
                    if k not in room:
                        room[k] = room_details[k]
                room["room_id"] = room["id"]
                self._available_rooms[room["name"]] = Room(**room)

        self.save("hipchat_rooms", self._available_rooms)
        if q:
            q.put(self._available_rooms)

Then, in usage:

@respond_to("history")
def test_room_history(self, message):
    room = self.get_room_from_message(message)
    print room.history

On the first bit, here's the notable things that are different:

  1. A proper Room class, based on Bunch.

    from will.utils import Bunch
    class Room(Bunch):
    
       @property
       def history(self):
           # history = requests() # make api request
           # return history
           return "Foo"
  2. self._available_rooms is now passed said Room object. (line 47)

    self._available_rooms[room["name"]] = Room(**room)

At least as a starting pattern, I'm curious how far this gets us. Obviously, we could also be tracking the history real-time as it comes in instead of calling the API endpoint, if performance is a concern. However, I'm not a fan of over-engineering based on "what if" needs. So, I'll leave that particular decision to you, based on your actual use-case.

Please let me know if you've got any questions, comments, or other thoughts!

-Steven

ghost commented 9 years ago

Ok I have a question. So in order to use will correctly the user will have done this:

export WILL_V2_TOKEN="...."

How do I get that token in python?

skoczen commented 9 years ago

Super easy!

from will import settings

settings.V2_TOKEN

:)

ghost commented 9 years ago

Oh.. I misspelled it as V2_Token and PyCharm is no help either...

skoczen commented 9 years ago

Yeah, there's some somewhat magical introspection/object creation that there's no chance PyCharm can see through. For reference, all env variables prefixed with WILL_ will show up in settings, as whatever the rest of their name is.

ghost commented 9 years ago

OK now I have a design question. By default, the history will be a dictionary with tons of data. I have a function that strips everything except for the message itself, the sender and the date, but I'm wondering if there may be some people who need more. So the question is, should the data be a pre-formatted or raw?

skoczen commented 9 years ago

Good question.

How much data are we talking? Are image binaries included?

Broadly, I've tried with will to provide useful human interfaces, with a bit-harder-to-get-to access to the raw data if people really need it.

Maybe something like (and I'm shooting from the hip here, your thoughts welcome:)

room.history = [
  {
    'sender': 'xmpp_jid3321@hipchat.com', 
    'timestamp': datetime.datetime.now(),  #whatever the timestamp is, as a python datetime
    'message': u"foo_bar", 
    'raw_data': {"foo": "bar", "etc": "ack",} 
   },
  {
    'sender': 'xmpp_jid3321@hipchat.com', 
    'timestamp': datetime.datetime.now(), 
    'message': "foo_bar", 
    'raw_data': {"foo": "bar", "etc": "ack",} 
   },
]

What do you think? What would help you the most in writing your plugin?

ghost commented 9 years ago

I think that sender, timestamp and message only are much better than to include the raw data additionally. If we're talking about useful human interfaces, it should be like this:

room.history = [
  {
    'sender': 'Firstname Lastname', 
    'timestamp': datetime.datetime.now(),  #whatever the timestamp is, as a python datetime
    'message': u"foo_bar" 
   },
  {
    'sender': 'Firstname Lastname', 
    'timestamp': datetime.datetime.now(), 
    'message': u"foo_bar"
   },
]

This is my preferred way of doing it, and if you give me your OK I'll implement it.

skoczen commented 9 years ago

I'd probably prefer xmpp_jid for sender, since you can use it to get a user's full profile (including name, nick and a bunch more) via self.get_user_by_jid(), and it keeps things DRY. If you're going to be doing a bunch of lookups, you could also just turn sender into the full user object for every entry (though this does seem like a lot of overhead).

Re: not providing raw, how big is it? Can you paste a sample?

Thanks!

Also, it's getting late here, so if I don't reply quickly, it's because I've gone to bed - will get you a reply in the AM!

ghost commented 9 years ago

I need to look into the JID but I want to provide a sample, you can find it here: http://pastebin.com/iBuRNsue It took a bit because I had to censor the data and run it through a JSON formatter.

skoczen commented 9 years ago

Thanks for taking the time!

So, looking at that, that all seems super useful, actually, and not terribly verbose/memory intensive. The mentions I could see being quite helpful, and message id I could see being really critical for ensuring uniqueness.

Honestly, the only change I'd make looking at that is to just convert date to a python datetime.

Leaving it all in would also allow us to be robust to api changes on hipchat's side, and will make documentation easier/more maintainable - we can just point people to their docs.

Does that work for you?

ghost commented 9 years ago

OK I can see how that might be useful. But one thing to note is that "id" is not the entire xmpp id, just the part after the "_" but self.get_user_by_full_name() should still do the trick right?

skoczen commented 9 years ago

Honestly, off of the top of my head, I'm not 100% sure. However, you do also have self.get_user_by_full_name() if that doesn't do the trick, and more lookups (nick, etc) can be added pretty easily.

roster.py is where all that goodness lives.

skoczen commented 9 years ago

Ok, because that was a lazy-ass answer (sorry), looking into it, that won't work for the jid. However, just add this method to roster.py, and you can look up by the id you're getting back (hipchat's internal ID)

def get_user_by_hipchat_id(self, id):
    for jid, info in self.internal_roster.items():
        if info["hipchat_id"] == id:
            return info
    return None

It'd be good to have that method anyway.

ghost commented 9 years ago

Neat. I don't quite understand this yet, but it does work, and it returns quite useful information. Now I need to convert the dates.

skoczen commented 9 years ago

Yeah, honestly the internal roster stuff is a deep dark hole that I don't understand half the time, until I'm neck-deep in it. :)

Dates: assume you know already, but for link convenience (I have it bookmarked because I always forget the options), strptime.

That's it for me tonight, but will take a look in the morning!

ghost commented 9 years ago

So the code is ready. I tested it and it works just as expected. The question now is whether I should document it, and if so, where to put it.

skoczen commented 9 years ago

Hey @PrideRage,

Fantastic. Yes, please document. Docs for .history should go in builtins.md, and I think a simple example showing getting a room's history and one example of the data you get (with a link to hipchat's docs) should cover it. I'd slot it in between "Access settings and config" and "Parse Natural Time".

Thanks much for the thorough work!

skoczen commented 9 years ago

Oh, and for reference, the docs are run through mkdocs. You can see how they'll render here: http://skoczen.github.io/will/plugins/builtins/

ghost commented 9 years ago

Thanks @skoczen, I'll work on it right now. mkdocs doesn't work for me though,

jinja2.exceptions.UndefinedError: 'config' is undefined

I guess jinja2 and mkdocs don't want to be friends on my machine.

skoczen commented 9 years ago

Don't sweat the mkdocs build - if you can just get solid docs in, I'm happy to finesse.

Installing pip install -r requirements.dev.txt should get you able to run the doc builds, but YMMV. Thus far, I've really been the only one building them anyhow.

ghost commented 9 years ago

Do I need to append two spaces at the end of every line for the markdown to recognize a line break?

skoczen commented 9 years ago

You shouldn't. I have a vague memory about something like that for a particular format I wanted in one spot, but normal markdown should do. Don't sweat the formatting too much - happy to finagle if needed.

Thanks!

skoczen commented 9 years ago

This is out as 0.6.6 - thanks again for the great PR and communication - and most of all, for making will even better!

ghost commented 9 years ago

Thanks for being so awesome! Feels good to give something back to the community.

skoczen commented 9 years ago

Keep it coming! IMO, this is how open source is supposed to be.