Conversation
|
Hey there and thank you for opening this pull request! 👋 We require pull request titles to follow the Conventional Commits specification. This comment indicates a fix is needed to repair this. How do I fix this?For most of you, this means that the PR title should say either
depending on whether you are fixing an issue or doing something new, respectively. But some other types might be suitable too, see the list below:
Please adjust the proposed title and this check will resolve automatically! Detailed error messageNo release type found in pull request title "User timezone". Add a prefix to indicate what kind of release this pull request corresponds to. For reference, see https://www.conventionalcommits.org/ Available types:
|
vEnhance
left a comment
vEnhance
left a comment
There was a problem hiding this comment.
This should have some sort of unit testing.
I might also need to, like, think through time a bit more. I feel like the time zone thing is disorienting because right now, when you see a time on the website, it's either UTC, ET, or (after this commit) the configured time zone... it would be good for them to be consistent. Probably the right thing to do is remove any part of the website that automatically sets time in UTC once this commit lands. (Actually, that might just be the pset queue right now?)
vEnhance
left a comment
vEnhance
left a comment
There was a problem hiding this comment.
we should also resolve the merge conflicts
| help_text="Show previous unit petitions that you have requested. Useful to disable if you have far too many.", | ||
| default=True, | ||
| ) | ||
| timezone = models.CharField( |
There was a problem hiding this comment.
so i don't know if this is possible, but i think this would be better with CHOICES somehow if it could be done. basically i don't think it's good UX for a end-user to have to type the exact case-sensitive string "America/Los_Angeles"... I would be unable to remember that.
|
thanks for working on this btw (i know i'm only posting negative feedback, but that's just because only the negative feedback is actionable and i don't mean to imply that i don't appreciate the work being done and the improvements being made each iteration. sorry if i'm being too brusque.) |
Add timezone field to UserProfile that allows users to view all timestamps in their local timezone: - CharField with blank default (uses server time America/New_York when blank) - JavaScript auto-detection using Intl API to prefill detected timezone - TimezoneMiddleware activates user's timezone per-request using zoneinfo - All existing |date template filters automatically respect the activated timezone
Replace nested getattr with get_or_create for better readability, matching the pattern used in LastSeenMiddleware above it.
- Add timezone validation to prevent invalid values in database - Remove unnecessary timezone.deactivate() calls in middleware - Move JavaScript to proper scripts block - Change auto-fill to suggestion UX with dismissible alert - Remove explicit UTC timezone display from pset queue - Add comprehensive unit tests for timezone functionality - Validator tests for valid/invalid timezones - UserProfile model validation tests - Middleware tests for authenticated/unauthenticated users - Form integration tests
|
i collapsed the migration files into one and made some cosmetic edits however, i don't think the time zone selection works for me. it gcenerates a list so long that it gets truncated, and i can neither scroll through the list nor actually type anything. if i wanted to set my time zone to Korea Standard Time, for example, I don't actually see how I can do this. am i missing something? |
| except zoneinfo.ZoneInfoNotFoundError: | ||
| pass # Fall back to default timezone | ||
| response = self.get_response(request) | ||
| timezone.deactivate() |
There was a problem hiding this comment.
actually, now that i think about it, why is there a timezone.deactivate here?
There was a problem hiding this comment.
ok as far as I can understand, timezone.activate() sets Django timezone in request-local context, and that context can be reused by later requests on the same worker.
timezone.deactivate() clears it so the next request doesn’t accidentally inherit another user’s timezone and instead falls back to settings.TIME_ZONE.
An alternative is not to set a global request timezone at all; convert datetimes only where needed in templates.
2 participants
Add a user timezone field
OTIS-WEB: dean