Code review comment for lp:~gary-lasker/software-center/recommender-feedback-viewed

Revision history for this message
Michael Vogt (mvo) wrote :

Thanks, that looks good and its great that this could get added quickly (and I like the test!).

Unfortunately the test fails for me with:
$ PYTHONPATH=. python tests/gtk3/test_catview.py RecommendationsTestCase
...
======================================================================
ERROR: test_implicit_recommender_feedback_on_item_viewed (__main__.RecommendationsTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gtk3/test_catview.py", line 276, in test_implicit_recommender_feedback_on_item_viewed
    self._populate_recommended_for_you_panel()
AttributeError: 'RecommendationsTestCase' object has no attribute '_populate_recommended_for_you_panel'

======================================================================
FAIL: test_recommended_for_you_spinner_display (__main__.RecommendationsTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gtk3/test_catview.py", line 162, in test_recommended_for_you_spinner_display
    self.rec_panel.recommended_for_you_content.get_property("visible"))
AssertionError: False is not true

----------------------------------------------------------------------
Ran 10 tests in 20.199s

One of the failures is probably releated to the lp:~gary-lasker/software-center/recommender-test-fix that I merged earlier and trivial to update. I have no idea about the other one, but it could be something that
is simply a side effect of the first.

When this branch needs to get touched anyway it would be nice to add a small comment to "on_application_activated" like "# send viewed items to the recommender in addition to installed ones" or similar. For a moment I was confused when looking at the code as it looks similar to the part in _on_transaction_finished (i.e. the difference of the two blocks is only visible when looking at the last part that has either "RecommenderFeedbackActions.INSTALLED" or RecommenderFeedbackActions.VIEWED). Or maybe extracting it into a common function?

« Back to merge proposal