Adding workday only tracking #18
Conversation
There was a problem hiding this comment.
This is dope! I'm so happy you're takin a crack at this. Hopefully you aren't cursing ramda by this point!
It looks like you're basically on the right track here, which is to say you've identified that countdowns can both be invoked manually or auto-reminded, so an event that users want business-day countdowns for needs to have that set on the countdown itself.
I wouldn't worry about testing manually at this point - it's gonna bog you down setting up your own test env for it tbh. Use the unit tests that exist. Once you have tests that pass how you expect them to, I'll take it for a test drive (since I have things set up here for a test env) and we can merge!
Last thing worth thinking about. Once you add a countdown, you might want to set business days only to true or false as an update. If it sounds complicated to add a new command for this or you don't wanna worry about it at this point, we can just set the business property once on countdown creation for now and I can add a new command for updating later using your work!
Thanks again - let me know if you have other questions. I applaud your effort since nobody but me has really tried pulling this repo down and getting it set up until you did. I'm not surprised (though I do apologize) that there were configuration issues that you had to figure out for the first time.
| ``` | ||
| git push heroku | ||
| ``` | ||
| 6. Issue a pull request from your fork back to the parent repository |
| "@babel/core": "^7.8.7", | ||
| "@babel/node": "^7.8.7", | ||
| "@babel/polyfill": "^7.8.7", | ||
| "@babel/preset-env": "^7.8.7", |
| export const CALLIE = 'callie'; | ||
| export const ALLOWED_FIELDS = ['date', 'token', 'teamId', 'event', 'destination', 'schedule', 'day', 'hour', 'countdown']; | ||
| export const CALLIE = process.env.CALLIE; | ||
| export const ALLOWED_FIELDS = ['date', 'token', 'teamId', 'event', 'destination', 'schedule', 'day', 'hour', 'countdown', 'workingdays']; |
There was a problem hiding this comment.
suggest business instead of workingdays to avoid the awkward no-space?
| import { notify, generateCountdownMessage } from '../services/message.service'; | ||
| import findCountdown from '../queries/findCountdown'; | ||
| import deleteCountdown from '../queries/deleteCountdown'; | ||
| import { isSuccessful } from '../utils/general'; | ||
|
|
||
| const setWeekDays = async (days) => { |
There was a problem hiding this comment.
is this actually async? there's no await in here
| [ R.T, setWeekDays([1, 2, 3, 4, 5, 6, 7]) ] | ||
| ]), | ||
| // Use the date prop which is a New moment() and find the diff with the current time in hours | ||
| date => date.businessDiff(moment().tz("America/Los_Angeles"), 'hours'), |
There was a problem hiding this comment.
tbh, i think setting the weekdays back and forth could be problematic because moment is an import and, for example, there could be many automated reminders at 10am on Monday that are going to be giving countdowns and could interfere with each other.
It's less ramda-y (which honestly this repo goes too far in that direction sometimes 😬) but what if we simplified and did something more like:
const getDaysUntilEvent = (event) => {
const { date, business } = event;
const eventMoment = moment(date);
const timeZone = moment().tz("America/Los_Angeles");
const hoursDiff = business ?
eventMoment.businessDiff(timeZone, 'hours') :
eventMoment.diff(timeZone, 'hours');
return Math.ceil(hoursDiff / 24);
}
| const isValidMessage = (messageEvent) => R.allPass([isChannelMessage, hasTeamId])(messageEvent); | ||
|
|
||
| // Used in isValidMessage |
There was a problem hiding this comment.
prob don't need to leave all these comments in?
| const createBaseConfiguration = async (messageEvent) => { | ||
| console.log(messageEvent); |
| const token = await getAccessToken(messageEvent); | ||
| return R.applySpec({ | ||
| channel: getChannel, | ||
| message: getEventMessage, | ||
| // TODO: get workingdays boolean from message string |
There was a problem hiding this comment.
so you'll basically do this in getSettingsAfterType. I'm gonna give you some pointers in the main review comment though.
This Pull request is not completely ready but it shows you the direction I am heading in.