skoczen / will

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

Reminder not working as intended #282

Closed wontonst closed 7 years ago

wontonst commented 7 years ago

image

Running inside a docker container, networked to a redis container. No errors on startup, warnings look fine.

Loading configuration...
  Importing config.py...
    ✓ Valid.
  Verifying settings...
    ! Warning: no HTTPSERVER_PORT found in the environment or config.  Defaulting to ':80'.
    - Note: WILL_REDIS_URL not set.  Defaulting to redis://localhost:6379/7.
    - Note: REDIS_MAX_CONNECTIONS not set. Defaulting to 4.
    ! Warning: no PUBLIC_URL found in the environment or config.  Defaulting to 'http://localhost:80'.
    ! Warning: no V1_TOKEN found in the environment or config.This is generally ok, but if you have more than 30 rooms, you may recieve rate-limit errors without one.

Latest pip install will

Tried switching to local file storage, doesn't make a difference.

I've also noticed that @periodic doesn't seem to work either.

skoczen commented 7 years ago

So, this is what I get:

screen shot 2017-09-29 at 2 52 42 pm

This strikes me as weird in two ways - one you're seeing extra responses from will, and two, it's not working. Have you added any custom methods?

wontonst commented 7 years ago

I have my own plugins but I don't see how they could affect the remind command - they're all simple responses to chat messages with nothing time-based. I didn't touch the prebundled plugins.

Here is the pip freeze

root@17d721617b06:/# pip freeze
APScheduler==2.1.2
args==0.1.0
bottle==0.12.7
CherryPy==3.6.0
clint==0.3.7
dill==0.2.1
dnspython==1.12.0
hiredis==0.2.0
Jinja2==2.7.3
Markdown==2.3.1
MarkupSafe==0.23
natural==0.1.5
parsedatetime==1.1.2
plumbum==1.6.3
pyasn1==0.1.7
pyasn1-modules==0.0.5
pygerduty==0.28
PyYAML==3.10
redis==2.10.3
requests==2.4.1
sleekxmpp==1.3.1
virtualenv==15.1.0
will==0.9.5

Might be some kind of dependency issue from my docker base image? Perhaps you can try to repro from a python:2.7 docker image.

The order of the responses vary as well, so something in schedule_say in reminder.py is not properly doing the scheduling.

self.schedule_say(formatted_reminder_text, parsed_time, message=message)
 self.say("%(reminder_text)s %(natural_datetime)s. Got it." % locals(), message=message)
wontonst commented 7 years ago

Example of one that went thru the other way around

image

wontonst commented 7 years ago

You know what, here you go, here's my dockerfile

FROM python:2.7

RUN bash -c '\
set -eux;\
pip install will will[hipchat] plumbum;\
mkdir /will_storage;\
generate_will_project;\
'
COPY config.py /config.py
COPY plugins/*.py /plugins/

ENV WILL_USERNAME=XXXX
ENV WILL_V2_TOKEN=XXXX

CMD '/run_will.py'
skoczen commented 7 years ago

The most likely cause of this is something funky with your date/time/timezone settings. Do you get immediate runs for more than a day in the future?

wontonst commented 7 years ago

Brilliant, that was spot on.

root@17d721617b06:/# date
Mon Oct  2 15:30:34 UTC 2017

image

Once I changed the timezone settings in the container remind worked perfectly.

Now, HipChat's room_message webhook call passes in an ISO8601 formatted date. I haven't played with it yet so I'm not sure if it includes time and timezone, but hypothetically if it does, somewhere in will, maybe will.listener._handle_message_listeners, you can grab that datetime info and store it in the message which would be the same message that's passed to plugins, so remind's pseudocode would look like

tz = extract_timezone(message.datetime)
remind_time = apply_timezone(natural_datetime, tz)
remind_time = convert_to_local_time_zone(remind_time)
self.schedule_say(reminder_text, remind_time, message=message)
skoczen commented 7 years ago

I'm a big +1 to sane, natural time zone handling.

In general, it's a complex space across platforms and distributed teams, but if you feel like there's specific ways Will could improve, I'd love to see an issue opened up to discuss it, or a PR!

It feels a bit out of scope for this particular issue, so I'm going to close this up - feel free to reopen if you disagree!