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

tz: check all the python now()s #2807

Closed
urapadmin opened this issue Aug 25, 2024 · 14 comments
Closed

tz: check all the python now()s #2807

urapadmin opened this issue Aug 25, 2024 · 14 comments
Assignees
Labels
kiosk a kiosk issue (not a filemaker issue)

Comments

@urapadmin
Copy link
Collaborator

they might rather need to be replaced with kioskdatetimelib.get_utc_now() .

@urapadmin urapadmin converted this from a draft issue Aug 25, 2024
@urapadmin urapadmin added the kiosk a kiosk issue (not a filemaker issue) label Aug 25, 2024
@urapadmin
Copy link
Collaborator Author

urapadmin commented Oct 21, 2024

  • ContextualFile etc. still has todos and they are not easily solved
  • SystemMessage and systemmessagestorepostgres are not using utc time stamps consistently. Smells like refactoring is needed. This has never been my best code.
  • postgresdbmigation._adapter_get_now_str and databasemigration.substitute_variables need thinking.
  • filemakerworkstation._set_workstation_constants still has a mysterious non-utc datetime.now()
  • KioskDatabaseIntegrity._get_functional_default_parameter needs thinking
  • What to do with all the default(now) in the dsd? Leave them?
  • locusrelationshook.py needs meticulous refactoring towards the new modified_tz/modified_ww fields
  • anc2kioskimport: same thing. Needs modified_tz / modified_ww fields

@urapadmin
Copy link
Collaborator Author

urapadmin commented Oct 21, 2024

To test:

  • adding, changing, deleting archaeological contexts to a file (in the file edit dialog). Check the modified fields!
  • file import, all sorts, including attaching them to arch contexts. Check the modified fields!
  • add files via sync -> modified?
  • update file via sync -> modified?
  • add file via file repos -> modified?
  • update file via file repos -> modified?
  • add file via upload file import
  • add file via bulk file import
  • add file via sequence import
  • does the recording app behave correctly in Summer Time / Winter Time?

@urapadmin
Copy link
Collaborator Author

postgresdbmigation._adapter_get_now_str is only used in databasemigration.substitute_variables. What it does is replace {NOW} with a current python now value.
It turns out we have only ONE script that uses the {NOW} variable and that is about creating the initial admin for the recording system in a new Kiosk. So that's fine.

This logs a warning from now on and it is also documented now.

@urapadmin
Copy link
Collaborator Author

KioskDatabaseIntegrity._get_functional_default_parameter needs thinking is used only in KioskDatabaseIntegrity. But it will indeed set all kinds of modified fields to the current utc-time stamp if they are null. That's still better than null and there isn't anything else I can do.
But the code creates a warning in the logs if it encounters that scenario.

@urapadmin
Copy link
Collaborator Author

urapadmin commented Oct 24, 2024

I could find default(now) only for coordinates in default_dsd3.yml. That's good. It should be discouraged. Is it really needed for coordinates? New ticket: #2926
There are a few more in dsd3_sync_subsystem but they all seem okay if they lead to a UTC time stamp. Nonetheless default(now()) should be used with care.

@urapadmin
Copy link
Collaborator Author

urapadmin commented Oct 24, 2024

repl_events aka as dock & synchronization events are UTC due to default("now()") and I think that's fine. The only other thing I could thing about is translating them into the displaying user's time zone. But that can wait.

@urapadmin
Copy link
Collaborator Author

anc2kioskimport: We take the timestamp for the modified field straight from the ANC database. Because we cannot know the time zone of that time stamp we treat it like any other legacy data from a former Kiosk version.

@luizaogs
Copy link
Collaborator

First bullet point issue: #2935

@urapadmin
Copy link
Collaborator Author

First bullet point issue: #2935

we have been testing the same list of checkboxes (which is good, we should double-check this).
Form my point of view it looks okay except for the site map problem with #2935

@urapadmin
Copy link
Collaborator Author

bulk and sequence is something I need to do on Beset itself. That's next.

@luizaogs
Copy link
Collaborator

What does summer vs winter time mean in this context?

@urapadmin
Copy link
Collaborator Author

What does summer vs winter time mean in this context?

I am not sure myself. It was more a mental note that there is such a thing as daylight savings time and that there might be a scenario where the shift could lead to additional complications. But I can't point my finger at anything concrete there.
Any effect would be limited to changes made within the hour of the time shift I guess. So this is pretty nuanced, I guess. Let's ignore it for now and keep it in the back of our heads.

@urapadmin urapadmin mentioned this issue Oct 29, 2024
2 tasks
@urapadmin
Copy link
Collaborator Author

What does summer vs winter time mean in this context?

I am not sure myself. It was more a mental note that there is such a thing as daylight savings time and that there might be a scenario where the shift could lead to additional complications. But I can't point my finger at anything concrete there. Any effect would be limited to changes made within the hour of the time shift I guess. So this is pretty nuanced, I guess. Let's ignore it for now and keep it in the back of our heads.

One thing just happened: I prepared a workstation for myself a few days ago. I downloaded it today (we changed the clock over night) and it won't start because the UTC offset is now different. That is correct and even better than simply comparing time zone names which would let me start the app but all the transformed modified dates in the app would be an hour off. So this is good. Just noting it here.

@urapadmin
Copy link
Collaborator Author

I close this. The last two (bulk and sequence) will be tested with #2924

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kiosk a kiosk issue (not a filemaker issue)
Projects
Status: done
Development

No branches or pull requests

2 participants