Unify ApplicationReview and TransitionLog models

[Anthony Lenton]

The ApplicationReview and TransitionLog models are very similar in essence.

The TransitionLog even has a comment field that could be used for the ApplicationReview's comment field. There is no user currently associated to the TransitionLog, but would make sense to be able to keep track of who exactly triggered each transition.

[Michael Nelson (michael.nelson)]

Some notes for a pre-imp discussion:

Uses of transition log:
      * Calculating Application.date_published
      * AppStateStatsExtractor.queryset_for_limits [1]
      * fix_app_transitions would be updated quite simply.
      * app_reviews_report would require simple changes - and could be moved to devportal.

Possible issues:
      * Currently an application review/comment can be added without changing the state, where as transition logs implicitly assume that there is one item per transition. This would affect things like the queries for apps in a certain state at a certain time.
         - Perhaps non-transitioning items should be identifiable - no state and no end_timestamp?

Other things
      * AppStateStatsExtractor could be moved to devportal quite easily.
      * Could almost remove all refs to Application from sca/stats.py - except for PaidAppsStatsExtractor... which checks the subscriptions related application's price... Why can't we just check the subscription's purchased_price? Probably campaigns? Perhaps we can include the price on the PublishedApplication? Or better, PublishedApplication.is_free_app?
      * Could also add TransitionLog.duration (instead of start timestamp) or similar, which would simplify some queries that collate to get duration average. Hrm, although there are querie using both timestamp and end_timestamp.

[1] Based on the usage, it looks like TransitionLog.end_state should really be TransitionLog.entered_state?

[Anthony Lenton (elachuni)]

Hi Michael,

Thanks for your thoughts... so you're thinking of removing ApplicationReview
 and leaving only TransitionLog, right?
I'm going to assume this and move on, I think it changes little if it turns out
 it was the other way around, anyway.

Michael Nelson wrote:
> Some notes for a pre-imp discussion:
>
> Uses of transition log:
> * Calculating Application.date_published
> * AppStateStatsExtractor.queryset_for_limits [1]
> * fix_app_transitions would be updated quite simply.
> * app_reviews_report would require simple changes - and
> could be moved to devportal.
>
> Possible issues:
> * Currently an application review/comment can be added
> without changing the state, where as transition logs implicitly
> assume that there is one item per transition. This would
> affect things like the queries for apps in a certain state at
> a certain time.
> - Perhaps non-transitioning items should be
> identifiable - no state and no end_timestamp?

Wouldn't it be ok to always allow state == end_state? That is, to transition
from a state back into the same state. This way transition-less comments
could be implemented by a transition back into the same state.
Also, the cases where we'd want to
allow transition-less comments are less than before, as we now always
move an app back into ReviewPending if a comment is made, if it was in
NeedsInformation... not that this changes anything, I guess.

> Other things
> * AppStateStatsExtractor could be moved to devportal quite
> easily.

+1

> * Could almost remove all refs to Application from
> sca/stats.py - except for PaidAppsStatsExtractor... which
> checks the subscriptions related application's price... Why
> can't we just check the subscription's purchased_price?
> Probably campaigns? Perhaps we can include the price on the
> PublishedApplication? Or better,
> PublishedApplication.is_free_app?

+1 to move price into PublishedApplication... isn't it there already?
sca will need it there when Subscriptions refer to PublishedApplication
instead of Application surely, as it needs that bit of data to initialize
the payment.

> * Could also add TransitionLog.duration (instead of start
> timestamp) or similar, which would simplify some queries that
> collate to get duration average. Hrm, although there are querie
> using both timestamp and end_timestamp.

Yep, I couldn't think of a way to simplify all queries with only
two of timestamp/end_timestamp/duration. We could add all three,
but that would imply redundancy in our data. I think things
aren't all that bad atm, if in doubt the policy could be to
make the least frequent use case do the extra math. :)

> [1] Based on the usage, it looks like TransitionLog.end_state
> should really be TransitionLog.entered_state?

Well, it is "The state the app is in at the end of this transition",
or "the state the app ends up in"... not what you had guessed at
first? :)

[Michael Nelson (michael.nelson)]

On Tue, Aug 28, 2012 at 9:29 PM, Anthony Lenton
<email address hidden> wrote:
> Hi Michael,
>
> Thanks for your thoughts... so you're thinking of removing ApplicationReview
> and leaving only TransitionLog, right?
> I'm going to assume this and move on, I think it changes little if it turns out
> it was the other way around, anyway.

I hadn't thought one way or the other yet.. but yep, it changes little here :)

> Michael Nelson wrote:
>> Possible issues:
>> * Currently an application review/comment can be added
>> without changing the state, where as transition logs implicitly
>> assume that there is one item per transition. This would
>> affect things like the queries for apps in a certain state at
>> a certain time.
>> - Perhaps non-transitioning items should be
>> identifiable - no state and no end_timestamp?
>
> Wouldn't it be ok to always allow state == end_state? That is, to transition
> from a state back into the same state. This way transition-less comments
> could be implemented by a transition back into the same state.

So when a report asks how many apps entered the NeedsReview state
during Jul 2012, how would you filter out those apps that didn't enter
NeedsReview, but just happened to be already in that state a comment
added ("Why is this taking so long?" :P)? That's all I meant above -
having some way to ensure we can easily filter out non-transitioning
items from similar queries.

>
>> * Could almost remove all refs to Application from
>> sca/stats.py - except for PaidAppsStatsExtractor... which
>> checks the subscriptions related application's price... Why
>> can't we just check the subscription's purchased_price?
>> Probably campaigns? Perhaps we can include the price on the
>> PublishedApplication? Or better,
>> PublishedApplication.is_free_app?
>
> +1 to move price into PublishedApplication... isn't it there already?

It's in the PublishedApplication.published_json, but isn't extracted
to a (db-searchable) attribute, like archive_id.

> sca will need it there when Subscriptions refer to PublishedApplication
> instead of Application surely, as it needs that bit of data to initialize
> the payment.

Right, but for that it doesn't need to be a DB column, it's
extractable from the published_json cheaply. The above comment was
just because we,'ve got reporting queries which assume they can run DB
queries filtering on price.

>
>> * Could also add TransitionLog.duration (instead of start
>> timestamp) or similar, which would simplify some queries that
>> collate to get duration average. Hrm, although there are querie
>> using both timestamp and end_timestamp.
>
> Yep, I couldn't think of a way to simplify all queries with only
> two of timestamp/end_timestamp/duration. We could add all three,
> but that would imply redundancy in our data. I think things
> aren't all that bad atm, if in doubt the policy could be to
> make the least frequent use case do the extra math. :)

Yep, +1 to leave as is as long as there's no way to simplify all
queries with only 2 fields.

>
>> [1] Based on the usage, it looks like TransitionLog.end_state
>> should really be TransitionLog.entered_state?...