Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add params for startDate, endDate and calculation of all recorded hours. #89

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tuomastanner
Copy link

No description provided.

@tuomastanner tuomastanner requested a review from lauravuo March 19, 2019 12:41
Copy link

@lauravuo lauravuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The slack integration will most likely be broken due the new params being undefined.


const totalHours = calendar.getTotalWorkHoursSinceDate(range.start, range.end);
logger.info(`Total working hours from range start ${totalHours}`);

const result = analyzer.calculateWorkedHours(range.entries);
const result = analyzer.calculateWorkedHours(range.entries, !!calcAll);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double negation?

@@ -52,6 +52,7 @@ export const calcFlextime = async (message) => {
if (userId) {
logger.info(`Fetching data for user id ${userId}`);
const email = request.email || await slack.getUserEmailForId(userId);
const { startDate, endDate, calcAll } = request;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These will be always undefined since they are not parsed from command.

logger.info(`Calculating flextime for ${email}`);
const data = await app.calcFlextime(email);
const calcFlexTime = async (email, startDate, endDate, calcAll) => {
logger.info(`Calculating flextime for ${email} ${startDate} ${endDate} ${calcAll ? 'all hours included' : ''}`);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

calcAll? What is the purpose, !calcAll counts also hours marked on public holidays? Should you define a default value that is aligned with previous functionality?

Originally ignoring of public holidays was implemented on purpose, since a lot of people recorded hours afterwards and entered working hours also to public holidays which mixed up the billable hours calculation. So I probably wouldn't allow people marking hours on public holidays.


return {
entries: sortedRangeEntries,
start: startDate || new Date(sortedRangeEntries[0].date),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed on purpose? sortedEntries -> sortedRangeEntries

@@ -63,7 +63,7 @@ export default (config, http) => {
.description('Send monthly statistics to given email address.')
.action(generateStats);
program
.command('flextime <email>')
.command('flextime <email> [startDate] [endDate] [calcAll]')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Describe params in cli help text. Update README with relevant parts.

@buggedcom
Copy link

@tuomastanner Can we abandon this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants