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

Fix videos only showing in one content node #78

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

mrpau-eugene
Copy link
Contributor

@mrpau-eugene mrpau-eugene commented Dec 22, 2017

@benjaoming I haven't tested building the content packs yet but what I did was had a before and after with this one.

I wrote the before to a file and the after to another file. My only basis was the Intro to Arcsine video.

Inside the before, Intro to Arcsine only appears once. But now, it appears 5 times in different topic nodes.

Before:
screen shot 2017-12-22 at 11 58 21 am

After:
screen shot 2017-12-22 at 11 58 36 am

I'll try to build the content packs again. Hopefully it fixes most of the issues regarding "missing" videos 🤞

Also, thanks to @rtibbles for mentioning about this bug 👍

@benjaoming
Copy link
Contributor

@mrpau-eugene

Awesome work, and about time that this is fixed :)

I'm thinking that this might need some verification in KA Lite itself, for instance:

  1. Will the video now download 5 times if someone uses the Manage UI to download?
  2. Will content recommendation still work? (dunno why it shouldn't, but this could be necessary to investigate)
  3. Do we have places where we try to link to a topic through a video id?

Don't think that we should over-complicate this, maybe we are lucky and everything works... one way to assert this quickly would be to introduce some tests that supply a content db with duplicate video IDs and see if those tests pass.. and to ensure that already existing tests also have duplicate content IDs.

Regarding moving the Indian topics, is this fix a blocker? Because it's actually more urgent for announcing 0.17.4.

@mrpau-eugene
Copy link
Contributor Author

Will the video now download 5 times if someone uses the Manage UI to download?

I'm not quite sure how this one works though. Will check the source code and might as well do some manual testing.

Will content recommendation still work? (dunno why it shouldn't, but this could be necessary to investigate)

About this one, when I tested the 0.17.3, I can't seem to find any recommendations after watching a video.. so I think it wasn't working. I was getting an error when I didn't comment out curatedRelatedVideos..

Do we have places where we try to link to a topic through a video id?

Can you explain a little bit further about this? I don't understand what you meant here.

@benjaoming
Copy link
Contributor

benjaoming commented Dec 25, 2017

Do we have places where we try to link to a topic through a video id?

Can you explain a little bit further about this? I don't understand what you meant here.

Should have written youtube_id -- this would be a more general concern, I'm not thinking of any particular function. But if the same video appears multiple times then this ID stops being unique. We might wanna be a bit sensitive to that -- maybe have a quick scan around the codebase for youtube_id to see how it's being used?

@mrpau-eugene
Copy link
Contributor Author

@benjaoming I only found one that seems to be relevant to youtube_id

https://github.com/learningequality/ka-lite-content-packs/blob/master/contentpacks/utils.py#L754

I can't think of any reason why it was made this way that it removes duplicate youtube_ids..

@mrpau-eugene
Copy link
Contributor Author

@benjaoming finally.. I think we can release KA-Lite by next week since videos are appearing properly

KA (Upstream)
screen shot 2018-01-05 at 10 24 35 pm

KA-Lite
screen shot 2018-01-05 at 10 25 25 pm

@radinamatic would you mind testing this english content pack if you have time?
Here's the link: https://drive.google.com/open?id=1QWdnFMwFt50VgIrbhXZXmGOSebyhyheG

@radinamatic
Copy link
Member

Awesome job @mrpau-eugene! 👍

I'll grab the content pack for testing and give it a ride before the next week!

@radinamatic
Copy link
Member

@mrpau-eugene Looking much better! 👍

The only question I have is why are these subtopics appearing with French titles, when the content at the last level is in English...?

ubuntu16 04 3 start running - oracle vm virtualbox_283

ubuntu16 04 3 start running - oracle vm virtualbox_284

Should we maybe place them in Math - France to be consistent with the rest?

@mrpau-eugene
Copy link
Contributor Author

mrpau-eugene commented Jan 10, 2018

The only question I have is why are these subtopics appearing with French titles, when the content at the last level is in English...?

@radinamatic it's because those math topics were just appended to the topic nodes list making it appear at the very last part of the Math subtopics

Should we maybe place them in Math - France to be consistent with the rest?

Yeah, I agree. Will do this asap

@mrpau-eugene
Copy link
Contributor Author

@radinamatic I tried to rebuild the content packs again with the Math (France) but only the 2ème primaire was moved to Math (France). Kinda weird how inconsistent the data is.

@benjaoming
Copy link
Contributor

benjaoming commented Jan 12, 2018

@mrpau-eugene @radinamatic

Since these are 100% French topics, they should not be part of the English content database at all, not even just moved to a different subtopic... but entirely ignored.

Is it an error in upstream data? Or is there a new property in the dataset regarding the language?

@mrpau-eugene
Copy link
Contributor Author

@benjaoming I can't tell if it's an error from the upstream though. I think since the subtopics are english, KA treated it to belong to the english content.

Should we just ignore the french ones instead?

@benjaoming
Copy link
Contributor

@mrpau-eugene

If someone asked us to make a decision right now, I think we should exclude them. No one will navigate something with these French topic names, that's completely useless in the English content pack :)

The issue might have existed before, but in cases where -- because we ignored duplicates -- these topics just remained empty.

  • Is there any data property that can help to identify these topics as French? Or do we have to make special rules for their titles?

  • Can you find identical contents (videos) nested in other topics? Because then we can 100% safely ignore these topics.. and the issue is indeed probably just something that we never encountered because we didn't have duplicated contents before now.

  • Are the contents (videos) unique to these French labeled sub topics? How do the same paths display on khanacademy.org ? Can you find the slug somewhere by googling it?

I tried looking for site:khanacademy.org "comparing-groups-through-10-example" and it seems like it is supposed to live in "Kindergarten" lessons: https://www.khanacademy.org/math/cc-kindergarten-math/cc-kindergarten-counting/kindergarten-comparing-numbers/v/comparing-groups-through-10-example

The French topics seem empty on the English site, but I don't know based on which data property they exclude them :/

https://www.khanacademy.org/math/be-1ere-primaire/be-nombres/be-plus-grand-plus-petit-comparaison-de-quantits/sitemap.xml

@mrpau-eugene
Copy link
Contributor Author

@benjaoming

Is there any data property that can help to identify these topics as French? Or do we have to make special rules for their titles?

Seems like they don't have any data available that distinguishes them from being French aside from its title.

Can you find identical contents (videos) nested in other topics? Because then we can 100% safely ignore these topics.. and the issue is indeed probably just something that we never encountered because we didn't have duplicated contents before now.

I couldn't find any other contents when I checked. I think we can safely add these French topics to the blacklist.

Are the contents (videos) unique to these French labeled sub topics? How do the same paths display on khanacademy.org ? Can you find the slug somewhere by googling it?

They aren't unique to the french sub topics.

We had the same results while googling comparing-groups-through-10-example. However, by navigating to the main site, it also existed in https://www.khanacademy.org/math/early-math/cc-early-math-counting-topic/modal/v/comparing-groups-through-10-example

Note: Removing the modal word https://www.khanacademy.org/math/early-math/cc-early-math-counting-topic/v/comparing-groups-through-10-example redirects me to https://www.khanacademy.org/math/cc-kindergarten-math/cc-kindergarten-counting/kindergarten-comparing-numbers/v/comparing-groups-through-10-example

@mrpau-eugene
Copy link
Contributor Author

@benjaoming https://drive.google.com/open?id=1Mxiq0Iy5lAuESveKpuCvmXSHyIO7zg5h

The French topics have been blacklisted in this contentpack

@benjaoming
Copy link
Contributor

Sorry about the slow response, @mrpau-eduard -- have built the new content pack into an installer and will be testing it now.

@benjaoming
Copy link
Contributor

Have tested the content pack, and Math now looks very tidy :)

The .debs are built for the proposed PPA and available in an hour or so @radinamatic : https://launchpad.net/~learningequality/+archive/ubuntu/ka-lite-proposed/+packages

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.

3 participants