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

fix(userstatus): trigger absence start job even for absences starting in the past #47183

Closed
wants to merge 1 commit into from

Conversation

miaulalala
Copy link
Contributor

@miaulalala miaulalala commented Aug 12, 2024

Fixes #47058

Summary

The absence start job was not created for absences in the past - which could also mean an absence starting today (absence timestamp is midnight, but time for $now is larger).

Since absences that start in the past and are still ongoing should be considered nonetheless, absences are now only evaluated by their end date. if that is in the past, no jobs are created.

Checklist

… in the past

Signed-off-by: Anna Larch <anna@nextcloud.com>
@miaulalala miaulalala added the 3. to review Waiting for reviews label Aug 12, 2024
@miaulalala miaulalala requested a review from st3iny August 12, 2024 14:53
@miaulalala miaulalala self-assigned this Aug 12, 2024
Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

I'm not sure about this change.

I deliberately chose not to emit the start event when an absence period already started in the past. An app may always listen to OutOfOfficeScheduledEvent or OutOfOfficeChangedEvent to handle those cases. So that the OutOfOfficeStartedEvent is reserved for when a scheduled absence starts without user interaction.

In a sense this could be seen as a breaking change. Although I'm not sure if there actually is a use case that would break (at least in our code).

Comment on lines +91 to +99
$runJobAt = ($eventData->getStartDate() < $now) ? $now : $eventData->getStartDate();
$this->jobList->scheduleAfter(
OutOfOfficeEventDispatcherJob::class,
$runJobAt,
[
'id' => $absence->getId(),
'event' => OutOfOfficeEventDispatcherJob::EVENT_START,
],
);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$runJobAt = ($eventData->getStartDate() < $now) ? $now : $eventData->getStartDate();
$this->jobList->scheduleAfter(
OutOfOfficeEventDispatcherJob::class,
$runJobAt,
[
'id' => $absence->getId(),
'event' => OutOfOfficeEventDispatcherJob::EVENT_START,
],
);
$this->jobList->scheduleAfter(
OutOfOfficeEventDispatcherJob::class,
$eventData->getStartDate(),
[
'id' => $absence->getId(),
'event' => OutOfOfficeEventDispatcherJob::EVENT_START,
],
);

Can always use getStartDate() here as the job will be picked up if it is in the past IIRC.

@miaulalala
Copy link
Contributor Author

I'm not sure about this change.

I deliberately chose not to emit the start event when an absence period already started in the past. An app may always listen to OutOfOfficeScheduledEvent or OutOfOfficeChangedEvent to handle those cases. So that the OutOfOfficeStartedEvent is reserved for when a scheduled absence starts without user interaction.

In a sense this could be seen as a breaking change. Although I'm not sure if there actually is a use case that would break (at least in our code).

good point, that is indeed something to be considered. Although a case could be made that an event should be thrown regardless as it is indeed starting right now, so event listeners could do their thing.

@miaulalala
Copy link
Contributor Author

Superseded by #47201

@miaulalala miaulalala closed this Aug 13, 2024
@st3iny st3iny deleted the fix/ooo-timestamp-issues branch August 14, 2024 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: User status automation for OOO is not triggering / resetting correctly
2 participants