#226 Add WYSIWYG editor and solve urllib3/requests dependency error
Merged by jflory7. Opened by shraddhaag.
fedora-commops/ shraddhaag/fedora-happiness-packets WYSIWYG  into  master

Download 226.patch

This commit includes the following:

  1. Adds a WYSIWYG text editor using django-ckeditor.
  2. Resolve urllib3 dependency error requests 2.21.0 has requirement urllib3<1.25,>=1.21.1, but you'll have urllib3 1.25.2 which is incompatible.

The usage of all the formatting options is tested on email. The output looks like this on the website and same on email as well.

Metadata Update from @jflory7:
- Pull-request tagged with: PASSED, needs testing, new change, type - frontend, type - summer coding

Metadata Update from @jflory7:
- Request assigned

A few weeks ago, CVE-2019-11324 was opened on urllib versions 1.24.1 and earlier. Please bump this dependency to 1.24.2 to patch the CVE.

Just curious, what does this change?

I don't know if this is standard or not, but I prefer to put variables outside of a method after import statements at the top of the module. Since these are used for configuration, it would help to group all related configuration values at the top of the file for easier reading. You could group it with the PROJECT_DIR variable on line 12.

Metadata Update from @jflory7:
- Pull-request untagged with: needs testing
- Pull-request tagged with: needs changes

Hi @shraddhaag, nice work! :thumbsup: It's super exciting to see the WYSIWYG editor!

I left some feedback in-line, but I have a few other comments:

  1. Happiness Archive does not display rich text formatting
  2. Editor window should fit user's screen size (not sure about mobile devices?)
  3. Embedded images should always adjust for the user's screen size in email and Happiness Archive / confirmation screen

Screenshot examples of each are below:

Comment 1: Happiness Archive does not render rich-text formatting:

Comment 1: Happiness Archive does not render rich-text formatting

Comment 2: Editor window goes off-screen when sending a new Happiness Packet:

Comment 2: Editor window goes off-screen when sending a new Happiness Packet

Do you think you can figure these out? If we can make these improvements, the PR is ready to go. :grinning:

rebased onto 114b245d56f1f57c9379f859a48f1f76b7ab86c9

@jflory7 I integrated all the changes. Please let me know if I need to change anything else :)

Just curious, what does this change?

By default HTML is not escaped and so is displayed as text. We need to mark the field as safe so that Django renders it as HTML. It's detailed here in the documentation.

@shraddhaag It's generally not advisable to mark text as safe in Django if it is coming from a user input.

YOu can

From a security standpoint, a user can inject <script> elements into the input and this can lead to Cross Side Scripting

AFAIK, Django does not have any method by which you can render HTML and protect XSS.

You might be interested in Mozilla Bleach for this

rebased onto 816e744d583f383e78c6bd672682b8272d53708a

Hi @skamath! I've updated the PR with your suggestions. Please let me know if I need to make anymore changes :)

rebased onto c826e844aec63f9923dc1278a5484e3867f0a9dc

rebased onto a1ed1b27e447b545d9e829a9e1faa32f3a0e3782

Metadata Update from @jflory7:
- Pull-request untagged with: needs changes
- Pull-request tagged with: needs review
- Request assigned

I re-tested and everything looks good. :thumbsup: I think this is ready to go, but I'll let @skamath give a final review + merge.

@shraddhaag Excellent work!! :100: :100:

This PR LGTM, however I am unable to merge this. Maybe a permission issue.

1 new commit added

  • Remove opbeat as a requirement

@skamath Oops, your ACL's weren't updated. They are now. :smile:

I'm going to go ahead and merge this. Thanks @shraddhaag! :clapper:

Pull-Request has been merged by jflory7

Metadata Update from @skamath:
- Request assignee reset