[HNT-374] Add interest picker #790
Conversation
| MIN_INTEREST_PICKER_COUNT = 8 # Minimum number of items in the interest picker. | ||
|
|
||
|
|
||
| def apply_interest_picker(response: CuratedRecommendationsResponse) -> None: |
There was a problem hiding this comment.
what do you think about passing in the result of response.feeds.get_sections() here instead of the entire response?
that would allow skipping the if not response.feeds: check, and set this function up to return a value instead of modifying the response in place. this would also make testing a bit easier, as you can test the output of different (simpler) inputs.
There was a problem hiding this comment.
Yeah, I don't love passing in a complex object and modifying it in place.
The interest picker logic determines response.sections[].isInitiallyVisible and response.interestPicker. This function could accept the result of response.feeds.get_sections() and return an InterestPicker, but it would still need to modify the sections in place. Do you think that's preferable?
I could also extract 2 or 3 small functions to make apply_interest_picker more readable. Would that help?
| # Skip the rank for the interest picker. | ||
| new_rank += 1 | ||
| section.receivedFeedRank = new_rank | ||
| new_rank += 1 |
There was a problem hiding this comment.
i'm unclear on the purpose of this for block. i don't see sections used anywhere below. is there some by reference variable updating happening?
maybe this would be more clear if this function returned a value?
There was a problem hiding this comment.
section.receivedFeedRank = new_rank updates the rank of the sections, because the interest picker has been inserted between them. I can change it to return a CuratedRecommendationsFeed and InterestPicker.
| if enable_interest_picker: | ||
| assert data["interestPicker"] is not None | ||
| else: | ||
| assert data.get("interestPicker") is None |
There was a problem hiding this comment.
just curious - why is one assertion using .get and the other isn't?
There was a problem hiding this comment.
Good catch! This property is always present, so get is not necessary.
| if len(picker_sections) < MIN_INTEREST_PICKER_COUNT: | ||
| for section, _ in sections: | ||
| section.isInitiallyVisible = True | ||
| return feed, None |
There was a problem hiding this comment.
as there is quite a bit of logic in this one function, my advice here is to break this out into multiple functions that have small responsibilities. something like:
- set
isInitiallyVisibleon sections (basically lines 41-47 and 57-60). acceptssectionsand returnssections. doesn't do anything with interest picker. - create an
InterestPicker. acceptssectionsand returnsInterestPickerorNone. - update
receivedFeedRankforsections. acceptsInterestPickerand returnssections. ifInterestPickerisNone, just returnssectionsunmodified.
an approach like this encapsulates functionality into smaller, dedicated functions that are more easily unit tested.
There was a problem hiding this comment.
if this really has to be out today, you could consider shipping as-is and refactoring immediately after.
mmiermans
force-pushed
the
hnt-374-interest-picker
branch
from
8e3e811 to
443f1d6
Compare
| section.isInitiallyVisible = False | ||
|
|
||
|
|
||
| def _get_interest_picker_rank(sections: list[tuple]) -> int: |
There was a problem hiding this comment.
nice! i think it would be great to have unit tests on this.
| section.isInitiallyVisible = True | ||
| visible_count += 1 | ||
| else: | ||
| section.isInitiallyVisible = False |
There was a problem hiding this comment.
i think this logic belongs here:
if len(picker_sections) < MIN_INTEREST_PICKER_COUNT:
for section, _ in sections:
section.isInitiallyVisible = True
but instead of checking the length of picker_sections, you check the number of sections that have isInitiallyVisible = false.
also - why can't this return sections? i'm confused why sections is always being modified by reference...
There was a problem hiding this comment.
after this function completes, you can use the result to determine if you need to create an InterestPicker or not.
| return random.randint(1, 3) | ||
|
|
||
|
|
||
| def _renumber_sections(sections: list[tuple], picker_rank: int) -> None: |
There was a problem hiding this comment.
can this return a value instead of modifying sections by reference?
| @@ -468,6 +469,10 @@ async def fetch( | |||
| curated_recommendations_request.sections, response.feeds | |||
| ) | |||
|
|
|||
| if curated_recommendations_request.enableInterestPicker and response.feeds: | |||
| interest_picker = create_interest_picker(response.feeds.get_sections()) | |||
| response.interestPicker = interest_picker | |||
There was a problem hiding this comment.
this is probably a framework/app assumption i'm not familiar with, but just to check - it's okay to set response.interestPicker = None?
There was a problem hiding this comment.
Yeah, that's fine. The default value is None.
References
Description
Return an 'Interest Picker' from the curated recommendation endpoint, to allow clients to follow sections.
PR Review Checklist
Put an
xin the boxes that apply[DISCO-####], and has the same title (if applicable)[load test: (abort|skip|warn)]keywords are applied to the last commit message (if applicable)