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(dav): Make current ooo info time-dependent #41962

Merged
merged 1 commit into from Dec 5, 2023

Conversation

ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Dec 1, 2023

Summary

  • If there is an out of office absence info and it happens now -> return data
  • Else: return no data

Checklist

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

This will break the option for clients or potential admin features in the feature to show OOO in the form, etc.

Please let's not do this 🙈
We should check the applicable time in talk, it's on the way.

@ChristophWurst
Copy link
Member Author

OOF. But the endpoint was specifically designed to only return data if it's currently active.

Do we need two endpoints?

@nickvergessen
Copy link
Member

OOF. But the endpoint was specifically designed to only return data if it's currently active.

Docs don't mention this:

Get the currently configured out-of-office data of a user.

So

Do we need two endpoints?

I would say no, a simple date check on the returned date easily tells if it's applicable or not?

			const now = new Date()
			const firstDay = new Date(absence.firstDay)
			const lastDay = new Date(absence.lastDay)
			return (now > firstDay && now < lastDay)

Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Tested, works as expected, returning 404, if OoO is not in the effect yet (or expired)

Also wanted to ask about second endpoint. I understand that it might be useful for clients to get that data beforehand, but that's not the general case, I'd say

@nickvergessen

This comment was marked as outdated.

@ChristophWurst
Copy link
Member Author

I would say no, a simple date check on the returned date easily tells if it's applicable or not?

No, it's not, because you don't know the timezone of the other user.

Pushed another version. The old endpoint to get the absence data independent of time is back. In addition there is a timezone-aware endpoint that only returns data if the absence happens right now.

@nickvergessen
Copy link
Member

@ChristophWurst
Copy link
Member Author

ChristophWurst commented Dec 1, 2023

API changed another time for the new return type. Please let me know if it's usable.

Edit: give me 3mins. Forgot something.

Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

OpenAPI looks good

@ChristophWurst
Copy link
Member Author

/backport to stable28

@blizzz
Copy link
Member

blizzz commented Dec 4, 2023

🏓 on the failing linters, see @nickvergessen's comments

@nickvergessen
Copy link
Member

OpenAPI needs rebuild 🙈

* If there is an out of office absence info and it happens now -> return
  data
* Else: return no data

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
Copy link
Member Author

CI is happy now. @nickvergessen are you, too?

@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Dec 5, 2023
@nickvergessen nickvergessen merged commit fffbb06 into master Dec 5, 2023
48 of 50 checks passed
@nickvergessen nickvergessen deleted the fix/dav/check-current-ooo-time branch December 5, 2023 09:52
@blizzz blizzz mentioned this pull request Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug feature: dav
Projects
Development

Successfully merging this pull request may close these issues.

Only show out of office message when applicable
5 participants