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.

Sign up for GitHub

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

Jump to bottom

Move inline JS to regular file + other small fixes #174

Merged
merged 11 commits into from
Jul 14, 2023
Merged

Move inline JS to regular file + other small fixes #174

merged 11 commits into from
Jul 14, 2023

Conversation

benoit74
Copy link
Collaborator

@benoit74 benoit74 commented Jul 10, 2023 •
edited
Loading

Rationale

  • Fix Remove inline javascript to comply with some CSP #168 + other small issues
  • Enhance logging around HTTP errors
  • Stop retrying HTTP errors since in most cases, the error is not transient + might lead to user ban
  • Replace the example course in README.md with another one in CC-BY-SA-NC (in order to avoid issues like what happened to youtube-dl)

Changes

  • add a Creative Commons sample EDX course
  • move all inline Javascript to standalone files to comply with some CSP
  • no more retry for login + HTTP errors
  • display HTTP error responses or save them in a temporary file for analysis
  • pin markupsafe until we upgrade Jinja2
  • fix issue with video that do not need to be re-encoded and were marked as missing

@benoit74 benoit74 requested a review from rgaudin July 10, 2023 13:10
@benoit74
Copy link
Collaborator Author

Same question as kolibri, should we release it as 1.1.0 as soon as this is merged ?

@benoit74 benoit74 force-pushed the small_fixes branch 3 times, most recently from 1c82b39 to 53af7ac Compare July 11, 2023 08:18
rgaudin
rgaudin requested changes Jul 11, 2023
View reviewed changes
Copy link
Member

@rgaudin rgaudin left a comment

Choose a reason for hiding this comment

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

Thanks!!

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
openedx2zim/instance_connection.py Outdated Show resolved Hide resolved
openedx2zim/instance_connection.py Show resolved Hide resolved
openedx2zim/scraper.py Show resolved Hide resolved
requirements.txt Show resolved Hide resolved
openedx2zim/templates/assets/freetextresponse.js Outdated Show resolved Hide resolved
@benoit74 benoit74 requested a review from rgaudin July 13, 2023 06:37
rgaudin
rgaudin approved these changes Jul 13, 2023
View reviewed changes
Copy link
Member

@rgaudin rgaudin left a comment

Choose a reason for hiding this comment

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

LGTM ; thanks for #179

@benoit74
Change sample course URL to a course with a clear Creative Commons Li…
377dd3e 
…cense
@benoit74
Perform only one attempt on login page to avoid locking account + add…
cd83a84 
… more logging when something bad happens
@benoit74
Do not retry on HTTP errors, most of them are not transient and might…
2f1ead5 
… lead to a user/scrapper ban. Retry only on low level errors (e.g. connection timeouts, DNS errors, ...)
@benoit74
Fix issue when video does not need to be re-encoded
092b7e2
@benoit74
Pin markupsafe version until we upgrade Jinja2
2b35485
@benoit74
Fix URL to download Google Fonts (upstream change) + fail script on f…
7f87ded 
…irst error
@benoit74
Fix typo
4d9b4b1
@benoit74
Move all inline Javascript to standalone files to comply with some CSP
7d61f8e
@benoit74
Save big HTTP error responses in a temporary file instead of displayi…
c6c29a2 
…ng them on screen
@benoit74
Save API response for course xblocks in temporary file for debugging
5164ed7
@benoit74
Small fixes following review
773101d
@benoit74 benoit74 merged commit 9a0d70b into main Jul 14, 2023
@benoit74 benoit74 deleted the small_fixes branch July 14, 2023 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgaudin rgaudin rgaudin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove inline javascript to comply with some CSP
2 participants
@benoit74 @rgaudin