762536 - [shipping] add date parameter to l10n data exports, to support getting snapshots for appversions

Status: RESOLVED FIXED

(Webtools Graveyard :: Elmo, defect, P2)

Description

[Axel Hecht [:Pike]]

In a world without milestones, we still want to be able to get snapshots of the l10n shipping data. Let's add a ISO UTC datetime parameter to the data urls that take appversions, which should allow us to mimic most of what milestones do today.

Probably gonna be implemented by limiting the Action query with when__lte=datetime.

Comment 1

[Peter Bengtsson [:peterbe]]

Note-to-self:

1. Write new extra test for actions4appversions() and see if you can control the output by suppliing a different up_until date.

2. Use a form class to handle the input of SignoffDataView()

Comment 2

[Peter Bengtsson [:peterbe]]

Attachment: New optional parameter up_until

I felt a bit weak on the *actual* understanding of the logic (regarding tips and top-most statuses) you explained in principle on the whiteboard. 

Using a form as the input parsing means that for datetime fields it accepts 
https://github.com/django/django/blob/master/django/conf/global_settings.py#L370-383

Comment 3

[Laura Thomson :laura]

Axel, can you review please?

Comment 4

[Axel Hecht [:Pike]]

Comment on attachment 651484 [details] [diff] [review]
New optional parameter up_until

Review of attachment 651484 [details] [diff] [review]:
-----------------------------------------------------------------

I like the general direction of this, but there are a few things I'd like to see still.

I think the field can just subclass Field. Also, should we have test_forms.py to just test the form behavior? I guess you're implicitly testing it as part of test_views.py now, ymmv.

Also, kind-of general style, even in class-based views, I prefer to not rely on self.foo for behavior of methods, but to explicitly put the dependencies into the method signature. I think I like storing data in self, for no better reason than debugging, not sure if my other views have a consistent coding style there.

What bugs me most is that we don't have a timezone-aware way to specify cut-off dates, aka, up_until. Do you have an idea on how to add that?

::: apps/shipping/views/forms.py
@@ +5,5 @@
> +from django import forms
> +from shipping.models import Milestone, AppVersion
> +
> +
> +class ModelInstanceField(forms.fields.CharField):

I think this can just be forms.fields.Field?

::: apps/shipping/views/status.py
@@ +59,5 @@
>              raise RuntimeError("Expecting either ms or av")
>          return self.data_for_appversion(appver)
>  
>      def data_for_appversion(self, appver):
> +        return (accepted_signoffs(appver, up_until=self.up_until),)

Put up_until explicitly into the signature? I did that in other class-based view methods to have explicit dependencies for testing and such.

Comment 5

[Peter Bengtsson [:peterbe]]

Attachment: Now with proper tests on the form.

Despite the r+ I went ahead and obsolete for this patch instead. On the test_forms.py has changed. 

Regarding the timezone. It's a bitch. 
We don't store timezone information with our objects unfortunately. E.g. a randanactioninstance.when --> datetime.datetime(2012, 8, 17, 6, 20, 24, 828691)

Which means that it was 06:20 in America/Los_Angeles when that action took place. So, I guess that means it actually took place at 14:20 UTC. 

So, sadly when you pass `?up_until=2012-08-17%206:20:00` you have to be aware that that's related to US west coast time. Stinks a bit but never any easy solutions. 

At some point we should migrate all our data to UTC and timezone aware datetime instances.

Comment 6

[Peter Bengtsson [:peterbe]]

Here's a possible solution to the problem. We *could* allow people to type in things like this:
  up_until = 2012-08-17T06:20:00.81273-01:00

That means that they're asking for 6:20am in Berlin time. We could then figure out what timezone our timezone-un-aware timestamps are and thus apply an offset. 

We'd need to depend on the pytz module which will be able to tell us that "America/Los_Angeles" ==> +8 which means that if someone is asking for: `2012-08-17T06:20:00.81273-01:00` they actually mean 2012-08-17T15:20:00.81273 (i.e. +8 - (-1) hours offset)

To make this possible we'd also need to extend the DateTimeField on the form class to one that accepts a couple more date time formats, namely the RFC one shown above. 

It would create quite a lot of work and I'm sure we'll get the timezone arithmetic wrong once or twice before it's ready.

Comment 7

[Axel Hecht [:Pike]]

hrm. http://stackoverflow.com/questions/10065190/migrating-mysql-datetime-fields-from-django-1-3-to-django-1-4 ohoh. apparently we need to answer this question once more when we got to 1.4.

I see some obvious bugs with default=datetime.now instead of utcnow.

Comment 8

[Peter Bengtsson [:peterbe]]

Note-to-self: CHange the API so the input is ALWAYS in UTC. Internally we need to convert this according to settings.TIME_ZONE.

Comment 9

[Peter Bengtsson [:peterbe]]

Attachment: UTC on the API endpoint, local time internally

What this makes is that something like:
https://elmo/shipping/l10n-changesets?av=fx4.0&up_until=2011-06-03+14:00:00
becomes a filter on datetime.datetime(2011,6,3,22,0,0) when it does its thing in api.py

How do you like my trick for how to figuring out the offset without having to involve settings.TIME_ZONE (in forms.py)?

Comment 10

[Axel Hecht [:Pike]]

(In reply to Peter Bengtsson [:peterbe] from comment #9)
> Created attachment 655759 [details] [diff] [review]
> UTC on the API endpoint, local time internally
> 
> What this makes is that something like:
> https://elmo/shipping/l10n-changesets?av=fx4.0&up_until=2011-06-03+14:00:00
> becomes a filter on datetime.datetime(2011,6,3,22,0,0) when it does its
> thing in api.py
> 
> How do you like my trick for how to figuring out the offset without having
> to involve settings.TIME_ZONE (in forms.py)?

I'm afraid that trick doesn't do the right thing if the date we're asking for isn't in the same DST period as we're currently.

I also guess that this is going to make us return different data for you and for me when running locally.

I'm afraid (still) that we need to take pytz in to elmo to fix this.

Also, I just realized this moment that all the Action objects I create here locally in Berlin all have completely different contexts in time than the ones we create through the web interface with PDT/PST locally. I guess we'll just need to live with that, though.

Comment 11

[Peter Bengtsson [:peterbe]]

(In reply to Axel Hecht [:Pike] from comment #10)
> (In reply to Peter Bengtsson [:peterbe] from comment #9)
> > Created attachment 655759 [details] [diff] [review]
> > UTC on the API endpoint, local time internally
> > 
> > What this makes is that something like:
> > https://elmo/shipping/l10n-changesets?av=fx4.0&up_until=2011-06-03+14:00:00
> > becomes a filter on datetime.datetime(2011,6,3,22,0,0) when it does its
> > thing in api.py
> > 
> > How do you like my trick for how to figuring out the offset without having
> > to involve settings.TIME_ZONE (in forms.py)?
> 
> I'm afraid that trick doesn't do the right thing if the date we're asking
> for isn't in the same DST period as we're currently.
> 
I see what you're saying. It works for dates that are "nowish" but a -8 hour difference is different today compared to December last year. 
Hmm... let me see what pytz can offer us here. 

> I also guess that this is going to make us return different data for you and
> for me when running locally.
> 
How is that? Your settings.TIME_ZONE affects your `datetime.datetime.now` which we can convert to a number by comparing it with `datetime.datetime.utcnow`. It'll be universal no matter which time zone you're in. 

Anyway, the problem isn't what the UTC is NOW (or the other way around). The problem is that we need to figure out what the time was THEN.

> I'm afraid (still) that we need to take pytz in to elmo to fix this.
> 
> Also, I just realized this moment that all the Action objects I create here
> locally in Berlin all have completely different contexts in time than the
> ones we create through the web interface with PDT/PST locally. I guess we'll
> just need to live with that, though.

I don't get that. Do I need to? Are you referring to when you hook up your local install to the live mysql database? Yes, that's a bit weird then.

Comment 12

[Peter Bengtsson [:peterbe]]

Note-to-self: Here's how to convert a UTC datetime into a local time using pytz::

 def test(inp):
    inp = datetime.datetime.strptime(inp, FMT)
    print "INPUT", repr(inp)
    inp_dt = inp.replace(tzinfo=UTC)
    local = pytz.timezone("America/Los_Angeles")
    utc_dt = inp_dt.astimezone(local)
    utc = utc_dt.replace(tzinfo=None)
    print "OUTPUT", repr(utc)
    print "DIFF", inp - utc
    print

 test("2012-08-28 12:00:00")
 test("2012-01-28 12:00:00")

And the output is::

 INPUT datetime.datetime(2012, 8, 28, 12, 0)
 OUTPUT datetime.datetime(2012, 8, 28, 5, 0)
 DIFF 7:00:00

 INPUT datetime.datetime(2012, 1, 28, 12, 0)
 OUTPUT datetime.datetime(2012, 1, 28, 4, 0)
 DIFF 8:00:00

Let's get to work!

Comment 13

[Peter Bengtsson [:peterbe]]

Attachment: This feels much more solid

It's a bit mind boggling with timezones so what I did was that I fired off 
/l10n-changesets?av=fx4.0&up_until=2011-08-28+12:00:00
and then I put a print in api.py just before::

     if up_until:
        actions = actions.filter(when__lte=up_until)

And correctly, it converted it by subtracting the right amount of hours. Also, I tried with a January date and it then corrected the value of `up_until` correctly. Phew!

Comment 14

[Axel Hecht [:Pike]]

Comment on attachment 656144 [details] [diff] [review]
This feels much more solid

Review of attachment 656144 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great, thanks.

I'm wondering, what's the policy for putting stuff into vendor-local/src and vendor-local/lib? src being git includes, lib being dumps?

Also, I think we probably don't want to hard code pytz' version. Should we land that as a seperate patch, with a comment indicating the version that we take?

::: requirements/prod.txt
@@ +7,4 @@
>  django-appconf
>  versiontools
>  feedparser==5.1
> +pytz==2012d

Should we really specify the version on pytz? Sounds like we'd glue ourselves to potentially stale pytz data.

Comment 15

[Peter Bengtsson [:peterbe]]

(In reply to Axel Hecht [:Pike] from comment #14)
> Comment on attachment 656144 [details] [diff] [review]
> This feels much more solid
> 
> Review of attachment 656144 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks great, thanks.
> 
> I'm wondering, what's the policy for putting stuff into vendor-local/src and
> vendor-local/lib? src being git includes, lib being dumps?
> 

Right. src is only for git submodules.
There's no formal "rule" written down for what to pip install and what to git submodule. ...when there is a choice. 
One reason to opt for src (and thus git submodule) is generally when the dependency doesn't have a good setup.py story.

Comment 16

[[github robot]]

Commit pushed to develop at https://github.com/mozilla/elmo

https://github.com/mozilla/elmo/commit/7eee33d4f5c579192e6f78a1b2fa28bf5898a402
bug 762536 - l10n export data takes a 'up_until' parameter and validates input better, r=Pike