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

Added time zone management module #21246

Merged
merged 26 commits into from
Jan 19, 2023
Merged

Added time zone management module #21246

merged 26 commits into from
Jan 19, 2023

Conversation

PeterConijn
Copy link
Contributor

This module aims to provide methods to correctly calculate the offset between a datetime in a time zone from UTC and the offset between time zones. Other than the functions in the Type Helper codeunit in the base app, this module takes daylight saving time into account when applicable.

Additionally, the module provides indicators on whether a time zone supports daylight saving time or whether a given datetime falls within the daylight saving time period for a given time zone.

@pri-kise
Copy link
Contributor

@PeterConijn your pull request looks very promising.

It would be really useful in my opinion to provide a procedure that uses the current time zone to calculate the offset.
Additionally, have you thought about adding a wrapper for GetSystemTimeZones to give access to the available time zone ids?
(https://learn.microsoft.com/en-us/previous-versions/windows/embedded/ms912391(v=winembedded.11)?redirectedfrom=MSDN)
If I read the documentation correctly these names must not be always the same.
https://learn.microsoft.com/de-de/dotnet/api/system.timezoneinfo.getsystemtimezones?view=net-7.0

Now waiting for someone of microsoft.

@PeterConijn
Copy link
Contributor Author

It would be really useful in my opinion to provide a procedure that uses the current time zone to calculate the offset.

That is a good idea. I will look into it and add it to the PR

Additionally, have you thought about adding a wrapper for GetSystemTimeZones to give access to the available time zone ids?

A list of time zones is already handled by the Time Zone Selection module in the System App so it would be superfluous.

@pri-kise
Copy link
Contributor

@PeterConijn thanks for clarification. I must have missed this System Feature.

I guess you have to include your permissionset in on the system application basic permissonset

@NKarolak
Copy link

@pri-kise, as Peter indicated: Once this PR is merged, please remember to obsolete/move TypeHelper functions to the new module. Old code:
https://github.com/StefanMaron/MSDyn365BC.Code.History/blob/bcbdcfc434259c1bfc3708b55d62752c5b5fca26/BaseApp/Source/Base%20Application/TypeHelper.Codeunit.al#L328

When you search the Base app for further occurrences of "TimeZone", you will find even more procedures to obsolete, such as https://github.com/StefanMaron/MSDyn365BC.Code.History/blob/1c682b0af15e55f729253392b821e64244712e68/BaseApp/Source/Base%20Application/TransformationRule.Table.al#L378 (provided such function exists in the new module; I did not check).

@PeterConijn
Copy link
Contributor Author

The automated builds seem to fail on the object nos (I used the default 50100-50149 range). Maybe a noobie question, but is this something Microsoft will do or is that something expected of me? If the latter, I would like to know which object numbers I can use.

@pri-kise
Copy link
Contributor

pri-kise commented Dec 1, 2022

The best way is to wait for someone of microsoft and update the id's for the Tests and the new Objects afterwards.

@pri-kise
Copy link
Contributor

pri-kise commented Dec 6, 2022

@JesperSchulz could you please assign someone to this pull request?
To give feedback if this module can be accepted and to provide some new Ids, that can be used for this new module?

@JesperSchulz
Copy link
Contributor

I was hoping for other members of the community to review this code too ;-) I know this particular repository has a history of "partner develops, Microsoft reviews", but in the long run that won't scale. This PR is something that many partners over time have requested, and I was hoping to see some involvement from them.
Oh yeah, and it's year-end, which always means "go-time" for all Business Central developers - that may also play into, why we haven't looked at this PR yet ourselves ;-) But I promise to do so today/tomorrow! The ID problem is an easy one to fix - that I can do for sure. Stay tuned!

@JesperSchulz
Copy link
Contributor

@PeterConijn, if you grant me permissions to your repo, I can push the new IDs :-) Alternatively, you can update them yourself. I've reserved 8720 - 8724 for you.

@pri-kise
Copy link
Contributor

pri-kise commented Dec 8, 2022

@JesperSchulz do you have some Ids for the Test Codeunits too?

@JesperSchulz
Copy link
Contributor

132979 seems to be available, both for the PS and test :-)

Updated object numbers for test
Updated object numbers for test perm. set
Updated object ranges in app.json files
@PeterConijn
Copy link
Contributor Author

@PeterConijn, if you grant me permissions to your repo, I can push the new IDs :-) Alternatively, you can update them yourself. I've reserved 8720 - 8724 for you.

Thanks @JesperSchulz. I have done both. You have contributor access to my fork and I have updated the object numbers in the app, test, and test permissions.

Updated object ranges in app.json file
@PeterConijn PeterConijn dismissed stale reviews from pri-kise and adrogin via abac627 January 3, 2023 08:22
@JesperSchulz
Copy link
Contributor

This is coming together nicely! I think I'll try to send this code through our internal build system tomorrow to see what it says 🥳

@JesperSchulz
Copy link
Contributor

Alright, that should do it. I made a few changes to the module locally (e.g. removed permissions and used inherent permissions instead). This module will ship with v. 22 in April!

@PeterConijn
Copy link
Contributor Author

Alright, that should do it. I made a few changes to the module locally (e.g. removed permissions and used inherent permissions instead). This module will ship with v. 22 in April!

That's great news! Thank you very much for your collaboration in this module.

@JesperSchulz JesperSchulz added the ships-in-future-update Fix ships in a future update label Jan 19, 2023
@JesperSchulz
Copy link
Contributor

Thanks for reporting this. We agree, and we’ll publish a fix asap, either in an update for the current version or in the next major release. Please do not reply to this, as we do not monitor closed issues. If you have follow-up questions or requests, please create a new issue where you reference this one.

Build ID: 52216.

@JesperSchulz
Copy link
Contributor

Merging the PR, even though it means the changes will get reverted with 21.4, as the module doesn't release until 22.0. But don't worry! The module will be back 🥳

@JesperSchulz JesperSchulz reopened this Jan 19, 2023
Copy link
Contributor

@JesperSchulz JesperSchulz left a comment

Choose a reason for hiding this comment

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

Approved. I've made some further changes in the internal PR, but most of the code stays as it is in this PR :-)

@JesperSchulz JesperSchulz merged commit e79d9c0 into microsoft:main Jan 19, 2023
aholstrup1 pushed a commit to aholstrup1/ALAppExtensions that referenced this pull request Sep 6, 2024
This module aims to provide methods to correctly calculate the offset
between a datetime in a time zone from UTC and the offset between time
zones. Other than the functions in the Type Helper codeunit in the base
app, this module takes daylight saving time into account when
applicable.

Additionally, the module provides indicators on whether a time zone
supports daylight saving time or whether a given datetime falls within
the daylight saving time period for a given time zone.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
processing-PR The PR is currently being reviewed ships-in-future-update Fix ships in a future update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants