Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

fix(filter:date) Timezone formatting issues. #1917

Closed

Conversation

latentflip
Copy link
Contributor

This commit fixes #1261 and #1532. This covers two separate issues:

  • Positive timezones were being formatted without a leading + resulting in a formatting string like: "HH:MM:ssZ" giving "12:13:141000" instead of "12:13:14+1000". Fixed by checking if timezone is > 0 and adding a leading "+".
  • Timezone output signs were inverted. mock.TzDate expects the timezone offset as it's first argument, not the timezone. This means that a mock.TzDate with a positive offset should result in a date string with a negative timezone,
    and vice-versa.

I've tried to stick to the style as much as possible. I have one question about this commit though: ISO_8601 states:

An offset of zero, in addition to having the special representation "Z", can also be stated numerically as "+00:00", "+0000", or "+00". However, it is not permitted to state it numerically with a negative sign, as "-00:00", "-0000", or "-00".

I have chosen that a UTC/0 offset timezone is represented with a Z, like: "12:13:14Z" as that seems to be more normal. Is that okay, or would "12:13:14+0000" be better for some reason?

I have signed the CLA, I am Philip Roberts from Scotland 😄

@jtymes
Copy link
Contributor

jtymes commented Jan 30, 2013

@latentflip your build is failing on a test - can you take a look? Thanks!

@latentflip
Copy link
Contributor Author

Sorry, will do! Pesky browsers!

@latentflip
Copy link
Contributor Author

Hmm, as best I can tell this is failing because I need to update the tests that are written within ngdoc, which I have done. This is because those previous tests were in fact incorrect.

So I have done that, but it seems that there is an angular-scenario.js file checked into testacular itself (https://github.com/testacular/testacular/blob/master/adapter/lib/angular-scenario.js#L19223) which is for an older version of angular, and fails on the test shown above.

Since travis appears to be running that file, which I can't update, I can't get travis to pass, even though my tests are all passing 😦

I am not totally au fait with angular's e2e runner, not testacular, so does anybody know if there is a reason that the above file is checked into testacular itself, that seems broken to me? @vojtajina, any ideas?

@latentflip
Copy link
Contributor Author

I think the above, although possibly still a question isn't what was tripping me up. Fairly sure I have a passing build now, as soon as Travis reemerges from the dead.

@latentflip
Copy link
Contributor Author

Alright this is passing now, and I am happy with it. It looks like I was wrong about the angular-scenario stuff above anyway.

Anyone got any complaints or is this ready to go?

@IgorMinar
Copy link
Contributor

holly cow! I'm shocked that we wrote this filter with these two bugs. good job fixing them!

simple repro: http://plnkr.co/edit/YbH516RiFCAyFLDp3cJh?p=preview

@IgorMinar
Copy link
Contributor

reviewed - needs more work, but only simple modifications. otherwise this looks good.

This commit fixes angular#1261 and angular#1532. This covers
two separate issues:

- Positive timezones were being formatted without
a leading `+` resulting in a formatting string
like: "HH:MM:ssZ" giving "12:13:141000" instead
of "12:13:14+1000". Fixed by checking if timezone
is > 0 and adding a leading "+".

- Timezone output signs were inverted.
mock.TzDate expects the timezone _offset_ as it's
first argument, _not_ the timezone. This means
that a mock.TzDate with a positive offset should
result in a date string with a negative timezone,
and vice-versa.
@latentflip
Copy link
Contributor Author

@IgorMinar thanks for the review! Have updated the commit as suggested and it's still passing. Anything else?

@latentflip
Copy link
Contributor Author

Bump: @IgorMinar anything else you need me to do on this to get it merged? Thanks.

@IgorMinar
Copy link
Contributor

PR Checklist (Minor Bugfix)

  • Contributor signed CLA now or in the past (if you just signed, leave a comment here with your real name)
  • PR doesn't introduce new api
  • [-] PR doesn't contain a breaking change
  • PR contains unit tests
  • [-] PR contains e2e tests (if suitable)
  • [-] PR contains documentation update (if suitable)
  • PR passes all tests on Travis (sanity)
  • PR passes all tests on ci.angularjs.org (cross-browser compatibility)
  • PR is rebased against recent master
  • PR is squashed into one commit per logical change
  • PR's commit messages are descriptive and allows us to autogenerate release notes (required commit message format)
  • All changes requested in review have been implemented

@IgorMinar
Copy link
Contributor

landed as b001c8e

thanks!

if you haven't received our t-shirt before and would like one please fill out this form: http://goo.gl/075Sj

@IgorMinar IgorMinar closed this Feb 7, 2013
@latentflip
Copy link
Contributor Author

Amazing. Thanks @IgorMinar

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Timezone offset bug
3 participants