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 support for markdown in vm and node notes field #214

Merged
merged 19 commits into from
Aug 4, 2017

Conversation

ricco386
Copy link
Member

@ricco386 ricco386 commented Jul 31, 2017

This way we can escape html, and more importantly javascript in order
to prevent XSS, but have nice formatable notes.

Functionality added due to issue #98

This way we can escape html, and more importantly javascript in order
to prevent XSS, but have nice formatable notes.
@ricco386 ricco386 added this to the 2.6.1 milestone Jul 31, 2017
@ricco386 ricco386 self-assigned this Jul 31, 2017
@dn0 dn0 modified the milestones: 2.6.2, 2.6.1 Jul 31, 2017
@ricco386 ricco386 changed the title Added ability to store text in markdown Added support for markdown in vm and node notes field Jul 31, 2017

@register.filter
def markdownify(text):
return markdown.markdown(text, safe_mode='escape')
Copy link
Member

Choose a reason for hiding this comment

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

Reading the documentation https://pythonhosted.org/Markdown/reference.html#markdown:

Copy link
Member Author

Choose a reason for hiding this comment

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

we dont mind safe_mode is deprecated as we will rely on django template escape, e.g. in template we escape the content than run via markdown and mark safe for django template to render.

@@ -7,6 +7,7 @@
from json import dumps
from datetime import datetime, timedelta

import markdown
Copy link
Member

Choose a reason for hiding this comment

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

Please move the import statement under import ipaddress

Our new template tag markdownify will handle escaping and also marking
string safe so there is no need to have it in template again.
This fix doesnt seems to break design on other places in our app.
We shall let users know that they can use markdown so I have updated
the field note.

When there is focus on the text area field it gets larger so is is
easier to edit the text.
@ricco386 ricco386 requested a review from secult August 3, 2017 13:08
@@ -13,3 +13,4 @@ simplejson==3.11.1
frozendict==1.2
requests==2.13.0
esdc-api==2.0.1
Markdown==2.6.8
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure that this should go to "requirements-both" and not to the "requirements-mgmt"?

Copy link
Member

Choose a reason for hiding this comment

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

Definitely requirements-mgmt.txt


try:
md = markdown.markdown(text)
except:
Copy link
Contributor

Choose a reason for hiding this comment

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

Too broad exception clause

Copy link
Member Author

Choose a reason for hiding this comment

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

We want too broad exception here, as we don't know what can get wrong in the markdown library and we don't want to have broken page in case text formatting in template tag got wrong...

Copy link
Contributor

Choose a reason for hiding this comment

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

Even in that case, you want to use "Exception" class, not everything (including SystemExit).

text = conditional_escape(text)

try:
md = markdown.markdown(text)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rename the md to something more meaningful

Due to renaming variable I have realized we cant have this bit of
code more readable by minor refactoring.
@ricco386 ricco386 modified the milestones: 2.6.1, 2.6.2 Aug 4, 2017

try:
md = markdown.markdown(text)
except:
Copy link
Contributor

Choose a reason for hiding this comment

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

Even in that case, you want to use "Exception" class, not everything (including SystemExit).

@ricco386 ricco386 requested a review from dn0 August 4, 2017 09:02
Changed javascript from hardcoded sizes to use new class for large
text area in modal window.

Regenerated whole scss files to css we use.
Added new class for markdownify table as other changes were breaking design.
ricco386 and others added 4 commits August 4, 2017 13:18
Updated template tag to be in one function and updated view details
to have markhown class that can be styled if necessary...
Update list to have some style as default inherited style for list was none
Added css to format lists nicely even nested ones.
Added extensions to understand tables and wrap code block.
@dn0 dn0 requested a review from secult August 4, 2017 13:02
@ricco386 ricco386 merged commit 77d6330 into master Aug 4, 2017
@ricco386 ricco386 deleted the 98-notes-markdown branch August 4, 2017 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants