Merge into trunk : recommended-installed-feedback : Code : Ubuntu Software Center

Reviewer	Review Type	Date Requested	Status
Michael Vogt (community)		2012-08-30	Approve on 2012-09-06
Review via email: mp+122154@code.staging.launchpad.net

Revision history for this message

Michael Vogt (mvo) wrote on 2012-08-28: Posted in a previous version of this proposal

#

On Tue, Aug 28, 2012 at 05:46:23AM -0000, Gary Lasker wrote:
> Gary Lasker has proposed merging lp:~gary-lasker/software-center/recommended-installed-feedback into lp:software-center with lp:~gary-lasker/software-center/update-to-latest-recommender-client as a prerequisite.

Thanks for working on this branch. I'm very happy to see this feature
coming.

[..]
> Note that this branch depends on the lp:~gary-lasker/software-center/update-to-latest-recommender-client branch as that branch adds the new recommender client code from lp:~canonical-ca-hackers/ubuntu-recommender/ubuntu-recommender-client. Also note that I made one small change to sreclient to fix the validate decorator for the pkgname argument to implicit_feedback so that it matches the validator for the other calls that use pkgname as an argument.

Thanks for this, if you fixed the sreclient_pristine.py, please also
send the diff to the server team (if you haven't done already) so that
they add it to their
lp:~canonical-ca-hackers/ubuntu-recommender/ubuntu-recommender-client
branch.

> Finally, I added a unit test for the new call in test_recagent.py, however like a few of the other tests there, the test is not currently working with the server. Therefore, it remains disabled as the other tests are. I'll look at this tomorrow to try to see what the issue is with these tests.
[..]

Maybe the staging server had problems. I just tested enabling them and
its working, but that may well be that its now fixed. While looking at
the test code I noticed that there is a bit of duplication in
there, I pushed a branch at
lp:~mvo/software-center/recagent-test-cleanup that removes most of
that.

I also noticed that the branch is missing a test for the new
functionality, it would be nice to add one to
tests/gtk3/test_recommendations_widget.py. Something like activate a
app, trigger a install transaction and verify that on finish the
self.recommender_agent.post_implicit_feedback() call was done.

> @@ -244,7 +262,7 @@
>
> def _on_submit_anon_profile_data(self, spawner,
> piston_submit_anon_profile):
> - self.emit("submit-anon_profile", piston_submit_anon_profile)
> + self.emit("submit-anon-profile-finished", piston_submit_anon_profile)

Nice that this is more consistent now (plus that it will actually work).

> === modified file 'softwarecenter/enums.py'
> --- softwarecenter/enums.py 2012-08-23 14:37:28 +0000
> +++ softwarecenter/enums.py 2012-08-28 05:45:23 +0000
> @@ -299,6 +299,9 @@
> LOBBY_RECOMMENDATIONS_CAROUSEL_LIMIT = 9
> DETAILS_RECOMMENDATIONS_CAROUSEL_LIMIT = 4
>
> +# action values for recommendations implicit feedback
> +RECOMMENDATIONS_FEEDBACK_INSTALLED = "installed"
> +

That looks like we could use:

class RecommenderFeedback:
INSTALLED = "installed"

(so that later "REMOVED" can be added there as well and its more
consitent with the rest of enums.py).

[..]
> def _on_application_activated(self, catview, app):
> self.emit("application-activated", app)
> + # we only track installed items of the user has opted-in to the
> + # recommendations service
> + if self.recommender_agent.i...

On Tue, Aug 28, 2012 at 05:46:23AM -0000, Gary Lasker wrote:
> Gary Lasker has proposed merging lp:~gary-lasker/software-center/recommended-installed-feedback into lp:software-center with lp:~gary-lasker/software-center/update-to-latest-recommender-client as a prerequisite.

Thanks for working on this branch. I'm very happy to see this feature
coming.

[..]
> Note that this branch depends on the lp:~gary-lasker/software-center/update-to-latest-recommender-client branch as that branch adds the new recommender client code from lp:~canonical-ca-hackers/ubuntu-recommender/ubuntu-recommender-client. Also note that I made one small change to sreclient to fix the validate decorator for the pkgname argument to implicit_feedback so that it matches the validator for the other calls that use pkgname as an argument.

Thanks for this, if you fixed the sreclient_pristine.py, please also
send the diff to the server team (if you haven't done already) so that
they add it to their
lp:~canonical-ca-hackers/ubuntu-recommender/ubuntu-recommender-client
branch. 
 
> Finally, I added a unit test for the new call in test_recagent.py, however like a few of the other tests there, the test is not currently working with the server. Therefore, it remains disabled as the other tests are. I'll look at this tomorrow to try to see what the issue is with these tests.
[..]

Maybe the staging server had problems. I just tested enabling them and
its working, but that may well be that its now fixed. While looking at
the test code I noticed that there is a bit of duplication in
there, I pushed a branch at
lp:~mvo/software-center/recagent-test-cleanup that removes most of
that.

I also noticed that the branch is missing a test for the new
functionality, it would be nice to add one to
tests/gtk3/test_recommendations_widget.py. Something like activate a
app, trigger a install transaction and verify that on finish the
self.recommender_agent.post_implicit_feedback() call was done.

> @@ -244,7 +262,7 @@
>  
>      def _on_submit_anon_profile_data(self, spawner,
>                                       piston_submit_anon_profile):
> -        self.emit("submit-anon_profile", piston_submit_anon_profile)
> +        self.emit("submit-anon-profile-finished", piston_submit_anon_profile)

Nice that this is more consistent now (plus that it will actually work).
  
> === modified file 'softwarecenter/enums.py'
> --- softwarecenter/enums.py	2012-08-23 14:37:28 +0000
> +++ softwarecenter/enums.py	2012-08-28 05:45:23 +0000
> @@ -299,6 +299,9 @@
>  LOBBY_RECOMMENDATIONS_CAROUSEL_LIMIT = 9
>  DETAILS_RECOMMENDATIONS_CAROUSEL_LIMIT = 4
>  
> +# action values for recommendations implicit feedback
> +RECOMMENDATIONS_FEEDBACK_INSTALLED = "installed"
> +

That looks like we could use:

class RecommenderFeedback:
      INSTALLED = "installed"

(so that later "REMOVED" can be added there as well and its more
consitent with the rest of enums.py).

[..]
>      def _on_application_activated(self, catview, app):
>          self.emit("application-activated", app)
> +        # we only track installed items of the user has opted-in to the
> +        # recommendations service
> +        if self.recommender_agent.is_opted_in():
> +            self.recommended_apps_viewed.append(app.pkgname)

self.recommended_apps_viewed should probably be a set (as we don't
want duplication and order is not important here).

Should self.recommended_apps_viewed we cleared when we get new data,
e.g on _on_app_recommendations_agent_refresh (and the equivalent
function for the categories). Or should this be persistent?  I'm don't
know the details of the spec well enough :) I guess it comes down to
if we should consider it a recommendation that are reached
"indirectly", e.g. click on it "app A", move back to main view search
for something else and then reach "app A" again later (without
following a recommendation) if installing that should be counted or
not. I don't really mind either way, but would like to know.

> +    def _on_transaction_started(self, backend, pkgname, appname, trans_id,
> +                                trans_type):
> +        if (trans_type != TransactionTypes.INSTALL and
> +            pkgname in self.recommended_apps_viewed):
> +            # if the transaction is not an installation we don't want to
> +            # track it as a recommended item
> +            self.recommended_apps_viewed.remove(pkgname)

Could the check for the transaction type simply be folded into the
_on_transaction_finished() ? So that there is only a single signal
type to watch?

> +    def _on_transaction_finished(self, backend, result):
> +        if result.pkgname in self.recommended_apps_viewed:
> +            self.recommended_apps_viewed.remove(result.pkgname)
> +            if network_state_is_connected(): 
> +                # no need to monitor for an error, etc., for this
> +                self.recommender_agent.post_implicit_feedback(
> +                        result.pkgname,
> +                        RECOMMENDATIONS_FEEDBACK_INSTALLED)

If the error condition does not matter, then we can probably also skip
the network check.

> === modified file 'tests/test_recagent.py'
> --- tests/test_recagent.py	2012-06-11 14:50:53 +0000
> +++ tests/test_recagent.py	2012-08-28 05:45:23 +0000
> +    @unittest.skip('disabled for now as the server is not quite working')
[..]

Nice update to use the skip decorator.

Cheers,
 Michael

Revision history for this message

Gary Lasker (gary-lasker) wrote on 2012-08-29: Posted in a previous version of this proposal

#

Just a note that I have a branch for review with the recommender-client changes that I mentioned above (and that have been made to sreclient_pristine.py). The merge proposal is here:

https://code.launchpad.net/~gary-lasker/ubuntu-recommender/ubuntu-recommender-client-implicit-feedback-tweaks/+merge/121730

Revision history for this message

Gary Lasker (gary-lasker) wrote on 2012-08-30: Posted in a previous version of this proposal

#

Download full text (7.3 KiB)

> On Tue, Aug 28, 2012 at 05:46:23AM -0000, Gary Lasker wrote:
> > Gary Lasker has proposed merging lp:~gary-lasker/software-center
> /recommended-installed-feedback into lp:software-center with lp:~gary-lasker
> /software-center/update-to-latest-recommender-client as a prerequisite.
>
> Thanks for working on this branch. I'm very happy to see this feature
> coming.
>
> [..]
> > Note that this branch depends on the lp:~gary-lasker/software-center/update-
> to-latest-recommender-client branch as that branch adds the new recommender
> client code from lp:~canonical-ca-hackers/ubuntu-recommender/ubuntu-
> recommender-client. Also note that I made one small change to sreclient to fix
> the validate decorator for the pkgname argument to implicit_feedback so that
> it matches the validator for the other calls that use pkgname as an argument.
>
> Thanks for this, if you fixed the sreclient_pristine.py, please also
> send the diff to the server team (if you haven't done already) so that
> they add it to their
> lp:~canonical-ca-hackers/ubuntu-recommender/ubuntu-recommender-client
> branch.

Yep, I proposed a branch for this and it has been merged by Lukasz.

>
> > Finally, I added a unit test for the new call in test_recagent.py, however
> like a few of the other tests there, the test is not currently working with
> the server. Therefore, it remains disabled as the other tests are. I'll look
> at this tomorrow to try to see what the issue is with these tests.
> [..]
>
> Maybe the staging server had problems. I just tested enabling them and
> its working, but that may well be that its now fixed. While looking at
> the test code I noticed that there is a bit of duplication in
> there, I pushed a branch at
> lp:~mvo/software-center/recagent-test-cleanup that removes most of
> that.
>
> I also noticed that the branch is missing a test for the new
> functionality, it would be nice to add one to
> tests/gtk3/test_recommendations_widget.py. Something like activate a
> app, trigger a install transaction and verify that on finish the
> self.recommender_agent.post_implicit_feedback() call was done.
>

I added a thorough test for this functionality in test_catview.py (this is where the other recommendations panel tests are, and imo it's consistent because the tests for the other lobby panels (What's New and Top Rated) are also located here. While in test_catview.py I did some cleanup and consolidation of duplicated code as well.

> > @@ -244,7 +262,7 @@
> >
> > def _on_submit_anon_profile_data(self, spawner,
> > piston_submit_anon_profile):
> > - self.emit("submit-anon_profile", piston_submit_anon_profile)
> > + self.emit("submit-anon-profile-finished",
> piston_submit_anon_profile)
>
> Nice that this is more consistent now (plus that it will actually work).
>
> > === modified file 'softwarecenter/enums.py'
> > --- softwarecenter/enums.py 2012-08-23 14:37:28 +0000
> > +++ softwarecenter/enums.py 2012-08-28 05:45:23 +0000
> > @@ -299,6 +299,9 @@
> > LOBBY_RECOMMENDATIONS_CAROUSEL_LIMIT = 9
> > DETAILS_RECOMMENDATIONS_CAROUSEL_LIMIT = 4
> >
> > +# action values for recommendations implicit f...

> On Tue, Aug 28, 2012 at 05:46:23AM -0000, Gary Lasker wrote:
> > Gary Lasker has proposed merging lp:~gary-lasker/software-center
> /recommended-installed-feedback into lp:software-center with lp:~gary-lasker
> /software-center/update-to-latest-recommender-client as a prerequisite.
> 
> Thanks for working on this branch. I'm very happy to see this feature
> coming.
> 
> [..]
> > Note that this branch depends on the lp:~gary-lasker/software-center/update-
> to-latest-recommender-client branch as that branch adds the new recommender
> client code from lp:~canonical-ca-hackers/ubuntu-recommender/ubuntu-
> recommender-client. Also note that I made one small change to sreclient to fix
> the validate decorator for the pkgname argument to implicit_feedback so that
> it matches the validator for the other calls that use pkgname as an argument.
> 
> Thanks for this, if you fixed the sreclient_pristine.py, please also
> send the diff to the server team (if you haven't done already) so that
> they add it to their
> lp:~canonical-ca-hackers/ubuntu-recommender/ubuntu-recommender-client
> branch.

Yep, I proposed a branch for this and it has been merged by Lukasz.

> 
> > Finally, I added a unit test for the new call in test_recagent.py, however
> like a few of the other tests there, the test is not currently working with
> the server. Therefore, it remains disabled as the other tests are. I'll look
> at this tomorrow to try to see what the issue is with these tests.
> [..]
> 
> Maybe the staging server had problems. I just tested enabling them and
> its working, but that may well be that its now fixed. While looking at
> the test code I noticed that there is a bit of duplication in
> there, I pushed a branch at
> lp:~mvo/software-center/recagent-test-cleanup that removes most of
> that.
> 
> I also noticed that the branch is missing a test for the new
> functionality, it would be nice to add one to
> tests/gtk3/test_recommendations_widget.py. Something like activate a
> app, trigger a install transaction and verify that on finish the
> self.recommender_agent.post_implicit_feedback() call was done.
>

I added a thorough test for this functionality in test_catview.py (this is where the other recommendations panel tests are, and imo it's consistent because the tests for the other lobby panels (What's New and Top Rated) are also located here. While in test_catview.py I did some cleanup and consolidation of duplicated code as well.

> > @@ -244,7 +262,7 @@
> >
> >      def _on_submit_anon_profile_data(self, spawner,
> >                                       piston_submit_anon_profile):
> > -        self.emit("submit-anon_profile", piston_submit_anon_profile)
> > +        self.emit("submit-anon-profile-finished",
> piston_submit_anon_profile)
> 
> Nice that this is more consistent now (plus that it will actually work).
> 
> > === modified file 'softwarecenter/enums.py'
> > --- softwarecenter/enums.py   2012-08-23 14:37:28 +0000
> > +++ softwarecenter/enums.py   2012-08-28 05:45:23 +0000
> > @@ -299,6 +299,9 @@
> >  LOBBY_RECOMMENDATIONS_CAROUSEL_LIMIT = 9
> >  DETAILS_RECOMMENDATIONS_CAROUSEL_LIMIT = 4
> >
> > +# action values for recommendations implicit feedback
> > +RECOMMENDATIONS_FEEDBACK_INSTALLED = "installed"
> > +
> 
> That looks like we could use:
> 
> class RecommenderFeedback:
>       INSTALLED = "installed"
>

Done! :)

> (so that later "REMOVED" can be added there as well and its more
> consitent with the rest of enums.py).
> 
> [..]
> >      def _on_application_activated(self, catview, app):
> >          self.emit("application-activated", app)
> > +        # we only track installed items of the user has opted-in to the
> > +        # recommendations service
> > +        if self.recommender_agent.is_opted_in():
> > +            self.recommended_apps_viewed.append(app.pkgname)
> 
> self.recommended_apps_viewed should probably be a set (as we don't
> want duplication and order is not important here).
>

Done! :)

> Should self.recommended_apps_viewed we cleared when we get new data,
> e.g on _on_app_recommendations_agent_refresh (and the equivalent
> function for the categories). Or should this be persistent?  I'm don't
> know the details of the spec well enough :) I guess it comes down to
> if we should consider it a recommendation that are reached
> "indirectly", e.g. click on it "app A", move back to main view search
> for something else and then reach "app A" again later (without
> following a recommendation) if installing that should be counted or
> not. I don't really mind either way, but would like to know.
>

Ok, so what happens is that when a person clicks on a recommended item, we remember that they did that for that item for as long as the current USC session is active (it is now, however, persisted between sessions, I think would be a mistake). I believe the way it works currently is optimal because I think that a person clicking on a recommended item probably became aware of that item *because* of the recommendation, and so we still want to report if that item is installed even if the install did not occur immediately after clicking through the recommendation. In other words, if the user looks at a recommended item, then navigates around a bit before revisiting (and installing) that recommended item, I think that should be considered a successful recommendation.

That's why it's designed as it is. If we decide this isn't exactly what we want, we can change it easily.

> > +    def _on_transaction_started(self, backend, pkgname, appname, trans_id,
> > +                                trans_type):
> > +        if (trans_type != TransactionTypes.INSTALL and
> > +            pkgname in self.recommended_apps_viewed):
> > +            # if the transaction is not an installation we don't want to
> > +            # track it as a recommended item
> > +            self.recommended_apps_viewed.remove(pkgname)
> 
> Could the check for the transaction type simply be folded into the
> _on_transaction_finished() ? So that there is only a single signal
> type to watch?

I wanted to do that! Definitely it would be preferable. However, I could not because (strangely) the transaction-finished signal does *not* include the transaction type. So, I had to catch the transaction-started signal also so that I could capture the type of the transaction.

> 
> > +    def _on_transaction_finished(self, backend, result):
> > +        if result.pkgname in self.recommended_apps_viewed:
> > +            self.recommended_apps_viewed.remove(result.pkgname)
> > +            if network_state_is_connected():
> > +                # no need to monitor for an error, etc., for this
> > +                self.recommender_agent.post_implicit_feedback(
> > +                        result.pkgname,
> > +                        RECOMMENDATIONS_FEEDBACK_INSTALLED)
> 
> If the error condition does not matter, then we can probably also skip
> the network check.

Yeah, it's just an added check to avoid the exception. No need to try to hit the server if we aren't connected, so I think it's ok. No?

> 
> > === modified file 'tests/test_recagent.py'
> > --- tests/test_recagent.py    2012-06-11 14:50:53 +0000
> > +++ tests/test_recagent.py    2012-08-28 05:45:23 +0000
> > +    @unittest.skip('disabled for now as the server is not quite working')
> [..]
> 
> Nice update to use the skip decorator.
> 
> Cheers,
>  Michael

Thanks again for your very detailed review and your suggestions!

Revision history for this message

Gary Lasker (gary-lasker) wrote on 2012-08-30: Posted in a previous version of this proposal

#

Oops, I noticed a typo in my last comment:

"...when a person clicks on a recommended item, we remember that they did that for that item for as long as the current USC session is active (it is now, however, persisted between sessions, I think would be a mistake)."

Should be (see the "*not*"):

"...when a person clicks on a recommended item, we remember that they did that for that item for as long as the current USC session is active (it is *not*, however, persisted between sessions, I think would be a mistake)."

Though maybe this was obvious, but I wanted to make sure that I'm clear.

Thanks again!

Revision history for this message

Gary Lasker (gary-lasker) wrote on 2012-08-30:

#

What the heck? Guess I won't "resubmit" a MP again. :/

Revision history for this message

Michael Vogt (mvo) wrote on 2012-08-31: Posted in a previous version of this proposal

#

Download full text (7.0 KiB)

On Thu, Aug 30, 2012 at 09:25:21PM -0000, Gary Lasker wrote:
> > On Tue, Aug 28, 2012 at 05:46:23AM -0000, Gary Lasker wrote:
> > > Gary Lasker has proposed merging lp:~gary-lasker/software-center
> > /recommended-installed-feedback into lp:software-center with lp:~gary-lasker
> > /software-center/update-to-latest-recommender-client as a prerequisite.
> >
> > Thanks for working on this branch. I'm very happy to see this feature
> > coming.
> >
> > [..]
> > > Note that this branch depends on the lp:~gary-lasker/software-center/update-
> > to-latest-recommender-client branch as that branch adds the new recommender
> > client code from lp:~canonical-ca-hackers/ubuntu-recommender/ubuntu-
> > recommender-client. Also note that I made one small change to sreclient to fix
> > the validate decorator for the pkgname argument to implicit_feedback so that
> > it matches the validator for the other calls that use pkgname as an argument.
> >
> > Thanks for this, if you fixed the sreclient_pristine.py, please also
> > send the diff to the server team (if you haven't done already) so that
> > they add it to their
> > lp:~canonical-ca-hackers/ubuntu-recommender/ubuntu-recommender-client
> > branch.
>
> Yep, I proposed a branch for this and it has been merged by Lukasz.

Great, thanks for this.

> > > Finally, I added a unit test for the new call in test_recagent.py, however
> > like a few of the other tests there, the test is not currently working with
> > the server. Therefore, it remains disabled as the other tests are. I'll look
> > at this tomorrow to try to see what the issue is with these tests.
> > [..]
> >
> > Maybe the staging server had problems. I just tested enabling them and
> > its working, but that may well be that its now fixed. While looking at
> > the test code I noticed that there is a bit of duplication in
> > there, I pushed a branch at
> > lp:~mvo/software-center/recagent-test-cleanup that removes most of
> > that.
> >
> > I also noticed that the branch is missing a test for the new
> > functionality, it would be nice to add one to
> > tests/gtk3/test_recommendations_widget.py. Something like activate a
> > app, trigger a install transaction and verify that on finish the
> > self.recommender_agent.post_implicit_feedback() call was done.
> >
>
> I added a thorough test for this functionality in test_catview.py (this is where the other recommendations panel tests are, and imo it's consistent because the tests for the other lobby panels (What's New and Top Rated) are also located here. While in test_catview.py I did some cleanup and consolidation of duplicated code as well.

Thanks for adding the test and for the cleanup there, that looks very
good!

[..]
> > Should self.recommended_apps_viewed we cleared when we get new data,
> > e.g on _on_app_recommendations_agent_refresh (and the equivalent
> > function for the categories). Or should this be persistent? I'm don't
> > know the details of the spec well enough :) I guess it comes down to
> > if we should consider it a recommendation that are reached
> > "indirectly", e.g. click on it "app A", move back to main view search
> > for something else and then reach "app A" again late...

On Thu, Aug 30, 2012 at 09:25:21PM -0000, Gary Lasker wrote:
> > On Tue, Aug 28, 2012 at 05:46:23AM -0000, Gary Lasker wrote:
> > > Gary Lasker has proposed merging lp:~gary-lasker/software-center
> > /recommended-installed-feedback into lp:software-center with lp:~gary-lasker
> > /software-center/update-to-latest-recommender-client as a prerequisite.
> > 
> > Thanks for working on this branch. I'm very happy to see this feature
> > coming.
> > 
> > [..]
> > > Note that this branch depends on the lp:~gary-lasker/software-center/update-
> > to-latest-recommender-client branch as that branch adds the new recommender
> > client code from lp:~canonical-ca-hackers/ubuntu-recommender/ubuntu-
> > recommender-client. Also note that I made one small change to sreclient to fix
> > the validate decorator for the pkgname argument to implicit_feedback so that
> > it matches the validator for the other calls that use pkgname as an argument.
> > 
> > Thanks for this, if you fixed the sreclient_pristine.py, please also
> > send the diff to the server team (if you haven't done already) so that
> > they add it to their
> > lp:~canonical-ca-hackers/ubuntu-recommender/ubuntu-recommender-client
> > branch.
> 
> Yep, I proposed a branch for this and it has been merged by Lukasz.

Great, thanks for this.
 
> > > Finally, I added a unit test for the new call in test_recagent.py, however
> > like a few of the other tests there, the test is not currently working with
> > the server. Therefore, it remains disabled as the other tests are. I'll look
> > at this tomorrow to try to see what the issue is with these tests.
> > [..]
> > 
> > Maybe the staging server had problems. I just tested enabling them and
> > its working, but that may well be that its now fixed. While looking at
> > the test code I noticed that there is a bit of duplication in
> > there, I pushed a branch at
> > lp:~mvo/software-center/recagent-test-cleanup that removes most of
> > that.
> > 
> > I also noticed that the branch is missing a test for the new
> > functionality, it would be nice to add one to
> > tests/gtk3/test_recommendations_widget.py. Something like activate a
> > app, trigger a install transaction and verify that on finish the
> > self.recommender_agent.post_implicit_feedback() call was done.
> > 
> 
> I added a thorough test for this functionality in test_catview.py (this is where the other recommendations panel tests are, and imo it's consistent because the tests for the other lobby panels (What's New and Top Rated) are also located here. While in test_catview.py I did some cleanup and consolidation of duplicated code as well.

Thanks for adding the test and for the cleanup there, that looks very
good!

[..]
> > Should self.recommended_apps_viewed we cleared when we get new data,
> > e.g on _on_app_recommendations_agent_refresh (and the equivalent
> > function for the categories). Or should this be persistent?  I'm don't
> > know the details of the spec well enough :) I guess it comes down to
> > if we should consider it a recommendation that are reached
> > "indirectly", e.g. click on it "app A", move back to main view search
> > for something else and then reach "app A" again later (without
> > following a recommendation) if installing that should be counted or
> > not. I don't really mind either way, but would like to know.
> > 
> 
> Ok, so what happens is that when a person clicks on a recommended item, we remember that they did that for that item for as long as the current USC session is active (it is now, however, persisted between sessions, I think would be a mistake). I believe the way it works currently is optimal because I think that a person clicking on a recommended item probably became aware of that item *because* of the recommendation, and so we still want to report if that item is installed even if the install did not occur immediately after clicking through the recommendation. In other words, if the user looks at a recommended item, then navigates around a bit before revisiting (and installing) that recommended item, I think that should be considered a successful recommendation.
> 
> That's why it's designed as it is. If we decide this isn't exactly what we want, we can change it easily.

Yeah, that is ok, I just wanted to raise it as a point of
consideration.

> > > +    def _on_transaction_started(self, backend, pkgname, appname, trans_id,
> > > +                                trans_type):
> > > +        if (trans_type != TransactionTypes.INSTALL and
> > > +            pkgname in self.recommended_apps_viewed):
> > > +            # if the transaction is not an installation we don't want to
> > > +            # track it as a recommended item
> > > +            self.recommended_apps_viewed.remove(pkgname)
> > 
> > Could the check for the transaction type simply be folded into the
> > _on_transaction_finished() ? So that there is only a single signal
> > type to watch?
> 
> I wanted to do that! Definitely it would be preferable. However, I could not because (strangely) the transaction-finished signal does *not* include the transaction type. So, I had to catch the transaction-started signal also so that I could capture the type of the transaction.

Indeed its not send! Its comming from
softwarecenter/backend/installbackend_impl/aptd.py so we could extend
it. But I guess its fine as it is.

> > 
> > > +    def _on_transaction_finished(self, backend, result):
> > > +        if result.pkgname in self.recommended_apps_viewed:
> > > +            self.recommended_apps_viewed.remove(result.pkgname)
> > > +            if network_state_is_connected():
> > > +                # no need to monitor for an error, etc., for this
> > > +                self.recommender_agent.post_implicit_feedback(
> > > +                        result.pkgname,
> > > +                        RECOMMENDATIONS_FEEDBACK_INSTALLED)
> > 
> > If the error condition does not matter, then we can probably also skip
> > the network check.
> 
> Yeah, it's just an added check to avoid the exception. No need to try to hit the server if we aren't connected, so I think it's ok. No?

Yeah, its ok t leave it there.

One tiny bit (that I can just changed during the merge) is that
"rec_item_tile.on_press()/rec_item_tile.on_release()" can be written
as "rec_item_tile.clicked()".

Its all looking good now, but I discovered a issue during my final
testing, I added:
=== modified file 'softwarecenter/backend/recagent.py'
--- softwarecenter/backend/recagent.py  2012-08-30 00:08:39 +0000
+++ softwarecenter/backend/recagent.py  2012-08-31 08:15:41 +0000
@@ -220,6 +220,7 @@
             "SoftwareCenterRecommenderAPI", "recommend_top")
 
     def post_implicit_feedback(self, pkgname, action):
+        print "++++++++++ implicit", pkgname, action
         # build the command
         spawner = SpawnHelper()
         spawner.parent_xid = self.xid

and clicked on a random item in the "Whats new" section of the
lobby (in my case ggcov) and I got on the terminal:
++++++++++ implicit ggcov installed

The same for items in the lobby in the top-rated section. Its 100%
reproducable for me. Please have a look if you see this as well.

Cheers,
 Michael

lp://staging/~gary-lasker/software-center/recommended-installed-feedback updated on 2012-09-05

3094. By Gary Lasker on 2012-08-31: add a test to insure that no recommender feedback call occurs in the case where the user is *not* opted in to the recommender service
3095. By Gary Lasker on 2012-08-31: merge with trunk
3096. By Gary Lasker on 2012-09-02: RED: add a failing test to show that the implicit recommender call is incorrectly being made from the top rated panel
3097. By Gary Lasker on 2012-09-03: the fix for the failing test case is to do the refactoring that we know is needed in CategoriesViewGtk to move the add_tiles_to_flowgrid and corresponding on_application_selected methods to where they belong, specifically, to the FlowableGrid class itself, this commit contains the brunt of this refactor, but there are some additional pieces needed to finish it up
3098. By Gary Lasker on 2012-09-04: GREEN: essential refactor complete and signals wired up, all test cases now working; removed the unused (with FIXME) 'application-selected' signal from catview as it is not used there (we only 'activate' apps from this widget), fix single-line cut-and-paste error in the test case
3099. By Gary Lasker on 2012-09-05: REFACTOR: make a new TileGrid subclass of FlowableGrid so that FlowableGrid remains generic, wire up the TileGrid for the various panels that it is used
3100. By Gary Lasker on 2012-09-05: REFACTOR: remove all need for the catview in the recommendations code
3101. By Gary Lasker on 2012-09-05: don't use hasattr in the FramedHeaderBox so that we can attach listeners to the more button when we need to, connect listeners for this, add a signal to the RecommenderPanelCategory (and subclass RecommenderPanelLobby) so that we can get their correct categories when clicking 'More' in those
3102. By Gary Lasker on 2012-09-05: pep8 fixes
3103. By Gary Lasker on 2012-09-05: merge with trunk
3104. By Gary Lasker on 2012-09-05: clean up get_test_window_recommendations in windows.py per the new factoring

Revision history for this message

Gary Lasker (gary-lasker) wrote on 2012-09-05:

#

Ok, I've added more unit test cases, done a lot of refactoring and cleanup around this area of the code (lots of FIXMEs cleared) and at this point the branch should be ready for review again. You can review the changes in detail by looking at the checkin comments starting at rev 3094.

Many thanks for your review, as always!!

Revision history for this message

Michael Vogt (mvo) wrote on 2012-09-06:

#

Hi Gary, thanks for this updated branch.

I just reviewed it and it looks very nice now. Some comments below:

I like that the recommendations widgets do no longer get a catview, giving them the db and propertieshelper
looks much nicer now.

395 class FlowableGrid(Gtk.Fixed):
396
397 MIN_HEIGHT = 100
398
399 + __gsignals__ = {
400 + "application-activated": (GObject.SignalFlags.RUN_LAST,
401 + None,
402 + (GObject.TYPE_PYOBJECT, ),
403 + ),
404 + }
405 +

It seems that the FlowableGrid is pretty generic (i.e. can hold other items than application tiles too) so this signal probably is better put into the TileGrid class? Or am I missing something here? I can do this during the merge if you agree with moving it.

I think its great that you added the docstring:
416 + def add_tiles(self, properties_helper, docs, amount):
417 + '''Adds application tiles to an ApplicationTileGrid:
418 + properties_help = an instance of the PropertiesHelper object
419 + docs = xapian documents (apps)
420 + amount = number of tiles to add from start of doc range'''
here! It might be worth taking a quick look http://www.python.org/dev/peps/pep-0257/ but there seems to be no real convention that everyone in the python community is following, so to a good extend its a matter of taste :) I personally like using a "--" as the keyword seperator there, e.g. "doc -- xapian docuemnts" better than using the "=" as initially my brain read it as code until I noticed the surrounding '''. But thats just a tiny detail and its good to have the docstring.

I like the way the "more" button is now handled, much nicer than in the previous code.

review: Approve

Revision history for this message

Gary Lasker (gary-lasker) wrote on 2012-09-06:

#

> Hi Gary, thanks for this updated branch.
>
> I just reviewed it and it looks very nice now. Some comments below:
>
>
> I like that the recommendations widgets do no longer get a catview, giving
> them the db and propertieshelper
> looks much nicer now.
>
> 395 class FlowableGrid(Gtk.Fixed):
> 396
> 397 MIN_HEIGHT = 100
> 398
> 399 + __gsignals__ = {
> 400 + "application-activated": (GObject.SignalFlags.RUN_LAST,
> 401 + None,
> 402 + (GObject.TYPE_PYOBJECT, ),
> 403 + ),
> 404 + }
> 405 +
>
> It seems that the FlowableGrid is pretty generic (i.e. can hold other items
> than application tiles too) so this signal probably is better put into the
> TileGrid class? Or am I missing something here? I can do this during the merge
> if you agree with moving it.

Yes! Thank you for noticing that. I just overlooked moving it down to the new subclass, but you are correct, that's where it belongs.

>
> I think its great that you added the docstring:
> 416 + def add_tiles(self, properties_helper, docs, amount):
> 417 + '''Adds application tiles to an ApplicationTileGrid:
> 418 + properties_help = an instance of the PropertiesHelper object
> 419 + docs = xapian documents (apps)
> 420 + amount = number of tiles to add from start of doc range'''
> here! It might be worth taking a quick look
> http://www.python.org/dev/peps/pep-0257/ but there seems to be no real
> convention that everyone in the python community is following, so to a good
> extend its a matter of taste :) I personally like using a "--" as the keyword
> seperator there, e.g. "doc -- xapian docuemnts" better than using the "=" as
> initially my brain read it as code until I noticed the surrounding '''. But
> thats just a tiny detail and its good to have the docstring.
>

I am with you completely. I just cut and pasted from another docstring and didn't notice the "=" signs. I would much prefer something different, and "--" seems just fine to me.

> I like the way the "more" button is now handled, much nicer than in the
> previous code.

Thanks! I like all of this better too. :) I'll make the updates now.

> Hi Gary, thanks for this updated branch.
> 
> I just reviewed it and it looks very nice now. Some comments below:
> 
> 
> I like that the recommendations widgets do no longer get a catview, giving
> them the db and propertieshelper
> looks much nicer now.
> 
> 395     class FlowableGrid(Gtk.Fixed):
> 396
> 397     MIN_HEIGHT = 100
> 398
> 399     + __gsignals__ = {
> 400     + "application-activated": (GObject.SignalFlags.RUN_LAST,
> 401     + None,
> 402     + (GObject.TYPE_PYOBJECT, ),
> 403     + ),
> 404     + }
> 405     +
> 
> It seems that the FlowableGrid is pretty generic (i.e. can hold other items
> than application tiles too) so this signal probably is better put into the
> TileGrid class? Or am I missing something here? I can do this during the merge
> if you agree with moving it.

Yes! Thank you for noticing that. I just overlooked moving it down to the new subclass, but you are correct, that's where it belongs.

> 
> I think its great that you added the docstring:
> 416     + def add_tiles(self, properties_helper, docs, amount):
> 417     + '''Adds application tiles to an ApplicationTileGrid:
> 418     + properties_help = an instance of the PropertiesHelper object
> 419     + docs = xapian documents (apps)
> 420     + amount = number of tiles to add from start of doc range'''
> here! It might be worth taking a quick look
> http://www.python.org/dev/peps/pep-0257/ but there seems to be no real
> convention that everyone in the python community is following, so to a good
> extend its a matter of taste :) I personally like using a "--" as the keyword
> seperator there, e.g. "doc -- xapian docuemnts" better than using the "=" as
> initially my brain read it as code until I noticed the surrounding '''. But
> thats just a tiny detail and its good to have the docstring.
>

I am with you completely. I just cut and pasted from another docstring and didn't notice the "=" signs. I would much prefer something different, and "--" seems just fine to me.

> I like the way the "more" button is now handled, much nicer than in the
> previous code.

Thanks! I like all of this better too. :) I'll make the updates now.

lp://staging/~gary-lasker/software-center/recommended-installed-feedback updated on 2012-09-06

3105. By Gary Lasker on 2012-09-06: move the application-activated signal into the TileGrid subclass where it belongs
3106. By Gary Lasker on 2012-09-06: make a docstring more better per review comment

Revision history for this message

Gary Lasker (gary-lasker) wrote on 2012-09-06:

#

Remaining small updates are done in the branch, now we just need the FFe! Bug 1044107. Hopefully very soon now.

lp://staging/~gary-lasker/software-center/recommended-installed-feedback updated on 2012-09-06

3107. By Gary Lasker on 2012-09-06: merge with trunk, don't wanna cruft

Ubuntu Software Center

Merge lp://staging/~gary-lasker/software-center/recommended-installed-feedback into lp://staging/software-center

Commit message

Description of the change

Preview Diff

Subscribers