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

Allow 0 hour tasks #603

Merged
merged 4 commits into from
Dec 20, 2022
Merged

Allow 0 hour tasks #603

merged 4 commits into from
Dec 20, 2022

Conversation

anarute
Copy link
Member

@anarute anarute commented Dec 3, 2022

Some users need to record 0-hour tasks to record when they work
or took the day off but don't want to affect the amount of worked hours.

In case a task starts at 00:00 and ends at 00:00, it means it
should be a 0-hour task instead of a 24hour task, so parse
the end hour from 00:00 to 24 only if the start time of the
task is not 00:00 too.
Some users need to record 0-hour tasks to record when they work
or took the day off but don't want to affect the amount of worked hours.
@eocanha
Copy link
Member

eocanha commented Dec 6, 2022

Thanks a lot for working on this!

Copy link
Member

@jaragunde jaragunde left a comment

Choose a reason for hiding this comment

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

I found an issue:

  1. Create an empty task with times 0:00 - 0:00
  2. Update init time to 1:00

At this point, save will fail with this error:

[Mon Dec 12 13:02:20 2022] [::1]:54134 [500]: POST /web/services/updateTasksService.php - Uncaught SQLQueryErrorException: ERROR:  23514: new row for relation "task" violates check constraint "end_after_init_task"
DETAIL:  Failing row contains (33, 2022-12-12, 60, 0, null, f, null, null, null, 2, 2, null, null, f, 2022-12-12 13:02:20.070846).
SCHEMA NAME:  public
TABLE NAME:  task
CONSTRAINT NAME:  end_after_init_task
LOCATION:  ExecConstraints, execMain.c:1934 in /var/www/html/phpreport/model/dao/TaskDAO/PostgreSQLTaskDAO.php:706
Stack trace:
#0 /var/www/html/phpreport/model/dao/TaskDAO/PostgreSQLTaskDAO.php(735): PostgreSQLTaskDAO->partialUpdate()
#1 /var/www/html/phpreport/model/facade/action/PartialUpdateTasksAction.php(132): PostgreSQLTaskDAO->batchPartialUpdate()
#2 /var/www/html/phpreport/model/facade/action/Action.php(112): PartialUpdateTasksAction->doExecute()
#3 /var/www/html/phpreport/model/facade/TasksFacade.php(154): Action->execute()
#4 /var/www/html/phpreport/web/services/updateTasksService.php(260): TasksFacade::PartialUpdateReports()
#5 {main}
  thrown in /var/www/html/phpreport/model/dao/TaskDAO/PostgreSQLTaskDAO.php on line 706

The problem is the end time is still 0:00 in the update operation. If you had created the task 1:00 - 0:00 from scratch, the ending 0:00 would have been transformed into 24:00.

model/facade/action/PartialUpdateTasksAction.php Outdated Show resolved Hide resolved
// (case it was a regular task and now it's a 0-hour task)
if ($newInitTime === 0 && $currentEndTime == 1440) {
$this->tasks[$i]->setEnd(0);
}
Copy link
Member

Choose a reason for hiding this comment

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

It's possible to call the update operation with no init time set, meaning that you don't want to update that field.

In that case, $newInitTime would be null. It looks like the code works properly because null is not bigger than $currentEndDate nor it equallls (===) zero, but I would wrap all this block in if $task->isInitTimeDirty() .... I would find it easier to understand what's going on and prevent exotic comparisons.

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense, I've just updated the last commit with this suggestion

In case it was a 0-hour task and not anymore, it means 00:00 is now
equal to 24h, the opposite means 00:00 really means 0.
// (case it was a regular task and now it's a 0-hour task)
if ($newInitTime === 0 && $currentEndTime == 1440) {
$task->setEnd(0);
}
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, a side effect of my last request is that this block doesn't run when you update the end time only. So, if you create a task from 0:00 to 1:00, save, then update end time to 0:00, it will still be recorded as a 24:00 task.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I've covered it now, hopefully I covered all scenarios 😅

@anarute
Copy link
Member Author

anarute commented Dec 20, 2022

@jaragunde gentle ping about this one :)

Copy link
Member

@jaragunde jaragunde left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. This looks good to me now, I didn't manage to break it 😉 . Thanks!

@anarute anarute merged commit 0d74451 into main Dec 20, 2022
@anarute anarute deleted the allow-0-hour-tasks branch December 20, 2022 14:02
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