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

Added JupterNotebookUploadQuestion. #701

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dzhuang
Copy link
Contributor

@dzhuang dzhuang commented May 20, 2020

Fix #690

@dzhuang dzhuang requested a review from inducer May 20, 2020 06:13
@codecov
Copy link

codecov bot commented May 20, 2020

Codecov Report

Merging #701 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #701      +/-   ##
==========================================
+ Coverage   96.82%   96.84%   +0.02%     
==========================================
  Files          45       45              
  Lines       11169    11245      +76     
  Branches     2058     2065       +7     
==========================================
+ Hits        10814    10890      +76     
  Misses        294      294              
  Partials       61       61              
Impacted Files Coverage Δ
course/page/__init__.py 100.00% <100.00%> (ø)
course/page/upload.py 100.00% <100.00%> (ø)
course/utils.py 99.33% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a89c621...d7b2974. Read the comment docs.

course/page/upload.py Outdated Show resolved Hide resolved

(body, resources) = html_exporter.from_notebook_node(notebook)

return "<div class='relate-notebook-container'>%s</div>" % body
Copy link
Owner

Choose a reason for hiding this comment

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

This is hard to review, as the code was moved. What was changed in addition to the move?

Copy link
Owner

Choose a reason for hiding this comment

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

I guess config_callback was added. Anything else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess config_callback was added. Anything else?

Yes. Because we need a different template (built-in basic.tpl template) as compared to what we do when we call the render_notebook_cells method, which uses the nbconvert_template.tpl.

I'll check if there's other changes.

:param clear_markdown: a :class:`bool` instance, indicating whether markdown
cells will be ignored..
:param config: a :class:`traitlets.config.loader.Config` instance.
:param config_callback: a function which further handles `config` .
Copy link
Owner

Choose a reason for hiding this comment

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

Be more precise here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try.

@@ -70,7 +70,7 @@
"hidden": False,
"listed": True,
"accepts_enrollment": True,
"git_source": "git://github.com/inducer/relate-sample",
"git_source": "git://github.com/dzhuang/relate-sample",
Copy link
Owner

Choose a reason for hiding this comment

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

This should be changed back before merge.

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.

return ext, b64decode(answer_data["base64_data"])


class FileUploadQuestion(FileUploadQuestionBase):
Copy link
Owner

Choose a reason for hiding this comment

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

This is hard to review, as much of this code was moved. Could you outline the changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll do that.

@inducer
Copy link
Owner

inducer commented Jun 3, 2020

Here's a pretty important question that occurred to me: Can you find/add a link in source to nbconvert's policy on whether it's designed to be safe with untrusted input?

@dzhuang
Copy link
Contributor Author

dzhuang commented Jun 4, 2020

Here's a pretty important question that occurred to me: Can you find/add a link in source to nbconvert's policy on whether it's designed to be safe with untrusted input?

I'll looking into that. At least, I found the notebook have the ability to execute Javascript scripts. I don't know whether that's still the case when its converted to html. That might be an issue before we can merge this PR.

@dzhuang
Copy link
Contributor Author

dzhuang commented Jun 4, 2020

@inducer
https://jupyter-notebook.readthedocs.io/en/stable/security.html#security-in-notebook-documents
That's the documentation about the security of notebook itself. I didn't found the docs for nbconvert.

@dzhuang
Copy link
Contributor Author

dzhuang commented Jun 4, 2020

There's a sanitize preprocessor, we need to make sure the preprocessor is included when we do the convert.

Base automatically changed from master to main March 8, 2021 02:16
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.

New type of question: JupyterNotebookQuestion
2 participants