Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature nrk add headliner #1770

Merged

Conversation

PatricLettau
Copy link
Contributor

Functional description

Substitute dysfunctional Recommended API by "Headliners" API endpoint, which provides a short list of recommended programs, typically what one would see highlighted very prominently on the website or app.

Reasoning

Recommendations have been broken for quite a while and the new API solves the same function of giving a short list of recommendations

Technical description

Parses output from the "Headliner" API-endpoint, which is structured differently then the rest of NRKs catalogue and creates a list of entries pointing to Series or directly to Videos.
Apparently this API is served by "something" different than the other APIs as the HTTP header is slightly malformatted. Instead of "," the cache-control directives are separated by ";", which needs a little workaround in cachehttpadater.

@PatricLettau
Copy link
Contributor Author

This is weird. Seems like coincidentally the categories API is not accessible any more.

@@ -122,6 +122,8 @@ def __extract_cache_data(self, headers):

if "cache-control" in headers:
cache_control = headers['cache-control']
#The "Headliner" NRK API endpoint uses ';' instead of ',' as a field delimiter
cache_control = cache_control.replace(";", ",")
Copy link
Collaborator

@basrieter basrieter Mar 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this change. They are not sticking to the HTTP specifications: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control#syntax

Multiple directives are permitted and must be comma-separated (e.g., Cache-control: max-age=180, public).

So I don't really feel like we should have code for that.

Copy link
Collaborator

@basrieter basrieter Mar 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I do see that things break now......So perhaps change the comment into something more generic: Some responses contain ';' as a delimiter. Fixing that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither do I. I already wrote them an email. Unfortunately without this "fix" caching doesn't work and throws exceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can change the comment to something more generic.

@basrieter
Copy link
Collaborator

I also noticed that the "categories" broke (that is why the tests break), but I have not been able to find the new API endpoint for them. Do you have any suggestions?

Copy link
Collaborator

@basrieter basrieter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you fixup the comment in the cache control?

@PatricLettau
Copy link
Contributor Author

About the categories, it seems like the currently used endpoint doesn't answer at all. There is another one which seems to be doing somewhat the same and is still online: https://psapi.nrk.no/documentation/redoc/pages-tv/v3.7/#tag/Pages
Haven't looked into the details yet but I can give it a shot

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@basrieter
Copy link
Collaborator

About the categories, it seems like the currently used endpoint doesn't answer at all. There is another one which seems to be doing somewhat the same and is still online: https://psapi.nrk.no/documentation/redoc/pages-tv/v3.7/#tag/Pages Haven't looked into the details yet but I can give it a shot

That is a start. I can find the categories, and list their content using https://psapi.nrk.no/documentation/redoc/pages-tv/v3.7/#tag/Pages/operation/GetGivenTVPage, however that result has too many sections to parse. So I wanted to use https://psapi.nrk.no/documentation/redoc/pages-tv/v3.7/#tag/Pages/operation/GetGivenSectionForTVPage to get each section separate, but it won't work.

@PatricLettau
Copy link
Contributor Author

This is a bit tricky indeed. This is what I found so far:

Get the list of categories from https://psapi.nrk.no/tv/pages/
The response size for each category page (https://psapi.nrk.no/tv/pages/{category} can be heavily limited by deactivating images and setting a limit to the number of returned sections (?limit=1&ImageProfile=NoWebImages). For the category drama-series this reduces the size from 707kb to 24kb. Really the only thing that is left in this response is a list of "Buttons", which link to subcategories. First "Button" in category drama-series points to "https://psapi.nrk.no/tv/pages/drama-serier/spenningsserier". This one returns 30 series, including titles, subtitles, images etc. and is "only" 51kb.

However, this only works for categories that actually have subcategories. Category "dyr" for example doesn't, even though it contains just over 100 programmes. I don't know if it's feasible to (re-)load the complete category page in case the list of Buttons is empty (for category "dyr" it's ~200kb with images and ~100kb without)

Going with subcategories will also add one level of hierarchy compared to what we had until now and I'm not too sure how stable those "Buttons" are, but I can't find another way to directly get all series in one category without parsing ~1MB per category for the larger ones.

Actually that's not entirely true. One could use "limit" and "offset" parameters for paging, but that gives quite unpredictable results as this limits "sections" which I have seen being anywhere between 1 title to ~20.

@basrieter
Copy link
Collaborator

I can always just flatten the hierarchy and add all results under a main category instead of the subsections.

@basrieter
Copy link
Collaborator

I am going to merge this, so I can work from here.

@basrieter basrieter merged commit 801e4de into retrospect-addon:master Mar 10, 2024
4 of 8 checks passed
@PatricLettau PatricLettau deleted the feature-nrk-add-headliner branch March 10, 2024 19:23
@basrieter
Copy link
Collaborator

@PatricLettau
Copy link
Contributor Author

Can you try https://github.com/retrospect-addon/plugin.video.retrospect/archive/refs/heads/nrk-cats.zip for the categories?

This works great! Couldn't find any issues with it.

@basrieter
Copy link
Collaborator

Great. Merged it. Will release tomorrow.

@PatricLettau
Copy link
Contributor Author

Thanks!

@basrieter basrieter added this to the v5.7.8 milestone Mar 14, 2024
@basrieter basrieter modified the milestones: v5.7.8, v5.7.10 Apr 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants