Skip to content

use ET timezone consistently in the admin UI#693

Open
jtmkrueger wants to merge 1 commit intomainfrom
695-et-tz
Open

use ET timezone consistently in the admin UI#693
jtmkrueger wants to merge 1 commit intomainfrom
695-et-tz

Conversation

@jtmkrueger
Copy link
Collaborator

Resolves 18F/api.data.gov#695

The code here utilizes the pre-existing format_iso8601_ms and removes the format_csv format. I suppose that a compromise could be made to create a new format that is more like the csv format being depricated, but I figured it'd be nicer to just have less formats to worry about and reuse one we're already using.

In the JS, just added some specifics to Parse the timezone as ET and add the timezone to the format string that's being shown to users.

Also adds a couple new tests for these changes.

Copy link
Member

@GUI GUI left a comment

Choose a reason for hiding this comment

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

@jtmkrueger, looks great overall! Just had one proposed tweak to the front-end code to respect a system-wide setting that can control the desired timezone. I'm hoping that session data should be available in this context, but holler if you run into any issues with that.

renderTime(value, type) {
if(type === 'display' && value && value !== '-') {
return moment(value).format('YYYY-MM-DD HH:mm:ss');
return moment(value).tz('America/New_York').format('YYYY-MM-DD HH:mm:ss z');
Copy link
Member

Choose a reason for hiding this comment

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

There's a system-wide analytics.timezone configuration setting that is used throughout the system to determine this type of default time zone to be used. This system setting is exposed during the login in some session data, and that's how the charts know what time zone to use:

let timezone = this.session.data.authenticated.analytics_timezone;

So to remain consistent with this configuration setting, I'd maybe recommend using that same setting here instead of hard-coding it to ET (even though for the api.data.gov deploy, it's hard-coded to ET):

Suggested change
return moment(value).tz('America/New_York').format('YYYY-MM-DD HH:mm:ss z');
return moment(value).tz(this.session.data.authenticated.analytics_timezone).format('YYYY-MM-DD HH:mm:ss z');

Comment on lines +309 to +329
def test_table_displays_time_in_eastern_timezone
FactoryBot.create(:log_item, :request_at => Time.parse("2015-01-16T06:06:28.816Z").utc, :request_method => "OPTIONS")
LogItem.refresh_indices!

admin_login
visit "/admin/#/stats/logs?search=&start_at=2015-01-12&end_at=2015-01-18&interval=day"
refute_selector(".busy-blocker")

assert_text("2015-01-16 01:06:28 EST")
end

def test_table_displays_time_in_eastern_timezone_during_dst
FactoryBot.create(:log_item, :request_at => Time.parse("2015-07-16T06:06:28.816Z").utc, :request_method => "OPTIONS")
LogItem.refresh_indices!

admin_login
visit "/admin/#/stats/logs?search=&start_at=2015-07-12&end_at=2015-07-18&interval=day"
refute_selector(".busy-blocker")

assert_text("2015-07-16 02:06:28 EDT")
end
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding this test coverage! Related to my last comment about leveraging the analytics.timezone setting, the test environment is hard-coded to use America/Denver:

analytics:
timezone: America/Denver

So if you start leveraging that setting, I think you'll need to update these tests to use MDT/MST for their frame of reference. There's technically ways you could also change the timezone in specific tests, but I think that's probably overkill here, so I think just updating these tests to verify the mountain time assumption is sufficient.

}, data)
end

def test_csv_timestamps_in_iso8601_utc_format
Copy link
Member

Choose a reason for hiding this comment

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

This test looks good, but there's one other semi-related test that's failing now: https://github.com/NatLabRockies/api-umbrella/actions/runs/21634777879/job/62359925312#step:5:385

@user1.created_at.utc.strftime("%Y-%m-%d %H:%M:%S"),
"2",
"2015-01-16 00:00:00",
@user1.use_description,
], csv[1])
assert_equal([
@user2.email,
@user2.first_name,
@user2.last_name,
@user2.website,
@user2.registration_source,
@user2.created_at.utc.strftime("%Y-%m-%d %H:%M:%S"),
"1",
"2015-01-17 00:00:00",

I think you'll need to update those tests to call user1.created_at.utc.iso8601 and then updated the hard-coded "2015-01-16 00:00:00" strings into ISO8601 format.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Clarify time zone in Analytics > Filter Logs screens

2 participants