search

praekelt / mama-ng-scheduler

A re-usable scheduler for MAMA Nigeria.
MIT License
0 stars 0 forks source link

Feature/issue 14 job for sending messages #16

Closed LyndsayLawrence closed 9 years ago

LyndsayLawrence commented 9 years ago

Ready for review - functional testing done.

hodgestar commented 9 years ago

Left some questions. Generally looks good. We desperately need unit tests for execute and postText.

LyndsayLawrence commented 9 years ago

Ready for re-review

hodgestar commented 9 years ago

Looking much nicer, thanks! Two things:

  1. We need to delete the schedule once it has fired for the last time and all associated messages have completed successfully. Currently we delete it in the wrong place. I think a sensible approach is something like:
    • Increment sendCounter as soon as the message is created.
    • Filter out schedules that have sendCounter equal to its maximum in the scheduler job.
    • Pass the message to postText and have that delete the schedule after it deletes a message if there are no more messages tied to the schedule and the schedule is complete.
  2. Now that the cron schedule for the scheduler and message jobs is configurable, I think it makes sense to make how far ahead in time the jobs look configurable too -- i.e. if the scheduler cron job is taking place every minute, it may make sense to only look one minute ahead, rather than one hour.
LyndsayLawrence commented 9 years ago

Ready for re-review

hodgestar commented 9 years ago

It doesn't look like anything saves or deletes a Message after the call to postText?

hodgestar commented 9 years ago

Left a few minor comments, otherwise looks good.

LyndsayLawrence commented 9 years ago

nothing in message changes when it gets executed and we don't delete it until it's delivery is confirmed (using /rest/messages/id DELETE)

LyndsayLawrence commented 9 years ago

Ready for re-review

hodgestar commented 9 years ago

So the URL being called needs to delete the message itself? Does it current get passed the ID of the message?

LyndsayLawrence commented 9 years ago

I'll be in the office tomorrow, then we can discuss this. It seems like we're on different pages.

hodgestar commented 9 years ago

Cool.

:+1: on landing this now and then we can discuss changes tomorrow.

hodgestar commented 9 years ago

Bit confused why this is closed rather than merged?

LyndsayLawrence commented 9 years ago

Not sure, I did "git flow feature finish " and this happened. The code is merged though.

hodgestar commented 9 years ago

Did you "git push" after "git flow feature finish"?

LyndsayLawrence commented 9 years ago

yes

hodgestar commented 9 years ago

I can see the last commit in develop so it is merged. Perhaps GitHub just got confused.