Code review comment for lp://staging/~canonical-ca-hackers/ubuntu-recommender/933881-expire-app-recommendation

Thanks for the overview of the app too!

10:03 < lukasz> noodles, wanna do a review?
10:07 < noodles> lukasz: sure, grabbing from email.
10:07 < noodles> lukasz: we can swap again? :)
10:07 < lukasz> noodles, sure
10:08 < noodles> lukasz: https://code.launchpad.net/~michael.nelson/software-center-a
gent/942664-part4-dry/+merge/95173
10:17 < noodles> lukasz: with your branch, my main question is why you're not adding
a date_created rather than an expired attribute?
10:18 < lukasz> noodles, how date_created would help with that?
10:20 < noodles> lukasz: the bug says that AppRecommendations should be expired after
 a certain time (if they have no feedback), whereas with the bool expired, you can't
(and don't) do that in your branch?
10:20 < noodles> Or is there already a date_created... I haven't checked.
10:20 < lukasz> noodles, I do, I mark them as expired which exclude them from being s
erved to users
10:21 < lukasz> noodles, no, there's no date_created (I'm going to add it in my next
branch)
10:21 < noodles> lukasz: you mark them as expired immediately - there's not time invo
lved is there? (As in, the app recommendation the command expires could have been cre
ated 1ms before)
10:21 < lukasz> noodles, for the bug which wants expired recommendations to be delete
d after a month
10:21 < lukasz> noodles, yes, idea is to run it daily
10:22 < noodles> lukasz: Oh, I was just reading the bug linked to your branch... and
assumed it mean that they should be deleted if they are one or two days old, rather t
han they should all be deleted every few days...
10:22 * noodles re-reads
10:23 < lukasz> noodles, last sentence in the description indicates that just deletin
g them is not a good idea
10:24 < lukasz> noodles, ironically my first stab at this bug had AppRecommendation.o
bjects(feedback__isnull=True).delete() :)
10:24 < noodles> lukasz: yes..., let me re-type the last part of my sentance: "rather
 than they should all be deleted every few days (the ones without feedback, that is)"
10:24 < noodles> heh :)
10:24 < noodles> lukasz: so I'm just thinking, do you even need an 'expired' attribute, if you add a date_created?
10:25 < lukasz> noodles, how would you decide that AppRecommendation which already exists should be ignored?
10:26 < noodles> lukasz: because you'd filter out apprecommendations older than X_DAYS (or optionally, only if they didn't have feedback)
10:28 < noodles> lukasz: another thought (sorry, I don't know the architecture well enough for recommender)... if a new recommendation comes in, and the only existing one is empty... why wouldn't you just re-use/update the existing one?
10:29 < lukasz> noodles, it's quicker to create new one than to update existing one
10:30 < noodles> lukasz: ok, but why not then *delete* the existing empty one when creating a new one, or is it useful data?
10:31 < lukasz> noodles, by new recommendation you mean new data which would change result of recommend_for_package?
10:32 < lukasz> noodles, if so, there's no link between AppRecommendation and any data which was used to create it. right now all of that data is processed offline
10:39 < noodles> lukasz: can we mumble in 10mins or so? I don't know the codebase well enough to get answers to the questions I've got, maybe we can just chat throguh it.
10:39 * noodles pops down to brinch the shopping up.
10:39 < lukasz> noodles, ~15 minutes would be better
10:47 < noodles> lukasz: sweet