-
Notifications
You must be signed in to change notification settings - Fork 2
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
add customization options to banner message #431
Conversation
Did you decide against making it expire? |
…age' into 426-frontend-improve-banner-message
Good point. I did decide against it, but then I took it as a challenge. now it expires. I doesn't disappear from a user's front end, but if they refresh or load the page after it expires, it wont be there anymore. This happens because now, everytime a user asks the server for the banner, it checks to see if its past expiration. if it is, it clears the banner from the database. |
It would be much better to read in the "America/Denver" string from a
configuration variable or slot in the database. This allows us to configure
it remotely, or deploy this more broadly with minimal code changes.
…On Mon, Sep 30, 2024 at 4:24 PM Michael Davenport ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/main/java/edu/byu/cs/controller/ConfigController.java
<#431 (comment)>
:
> + private static Instant getInstantFromUnzonedTime(String timestampString) throws DateTimeParseException {
+ ZoneId utahZone = ZoneId.of("America/Denver");
+
+ DateTimeFormatter formatter = DateTimeFormatter.ISO_LOCAL_DATE_TIME;
+ LocalDateTime localDateTime = LocalDateTime.parse(timestampString, formatter);
+ ZonedDateTime utahTime = localDateTime.atZone(utahZone);
+
+ return utahTime.toInstant();
+ }
Do we want to be hardcoding this? @frozenfrank
<https://github.com/frozenfrank> what do you think?
—
Reply to this email directly, view it on GitHub
<#431 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEXEBODN4SEM76BU3RXOXW3ZZHFRLAVCNFSM6AAAAABO3TBI2GVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMZYG4ZDINRVGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Is this something that we'll want to change across the app? Everything else in the autograder already assumes Utah time, at least as far as I'm aware |
@DallinFromEarth @19mdavenport This configuration improvement is really part of #156. While we have tried to minimize the number of times we hardcode the timezone, there currently appear to be 5 different occurrences of the timezone definition in the code. Several of them already have comments indicating that we should extract the exact value into a config table. I don't this we should hold up this PR on the config table improvements. We will already need to extract the constant from multiple other files in order to fully fulfill the request, and that isn't something we need to do now. However, that doesn't mean we can just assume that autograder is operating in the Utah timezone. We should practice technical diligence and avoid making decisions for as long as possible (good software design principles). |
you can now put a link into the banner message that is clickable and change the color