Enhancements on the freeform-answers report #17

iamjazzar · 2018-07-25T13:11:40Z

Description

This PR contains 3 different enhancements:

Horizantal line between questions:
Appears on all generated reports and there's no custom setting or flow controls it.
Default PDF header to Course Title:
Changes made on the display_name field where it's now left blank to determine whether to use the Course Title or another custom value.
Use custom fonts in generated PDF reports:
Users now can specify a url to the desired font and we will generate the report using it. The font can be hosted anywhere on the web, or can be uploaded on Studio. If the url is uploaded on Studio, the user should fill the custom_font field with the Web url not the ~~Studio~~ one.

Tests

Custom fonts tested.
PDF custom title tested.

Screenshot

mtyaka

Nice work @AhmedAljazzar! I tested this and it works well. I asked for a few changes - most importantly the title of the PDF should be a separate field, we should leave display_name unchanged. The other part is supporting /static/* Studio URLs for font files.

mtyaka · 2018-07-26T04:47:46Z

eoc_journal/eoc_journal.py

@@ -61,8 +63,8 @@ class EOCJournalXBlock(StudioEditableXBlockMixin, XBlock):

    display_name = String(
        display_name=_("Title (display name)"),
-        help=_("Title to display"),
-        default=_("Course Journal"),
+        help=_("Title to display. Leave blank to use the course title."),


@AhmedAljazzar After looking at related client tickets again, I think that they only want the title of the PDF to default to the course name, but not the block's display_name. I should have realized that and made it clear, sorry about that.
We will have to revert this change to the display_name field and add a separate setting pdf_title, which defaults to current course name but can be set to an arbitrary value by the author.

mtyaka · 2018-07-26T05:12:58Z

eoc_journal/eoc_journal.py

+    custom_font = String(
+        display_name=_("Default Font"),
+        help=_("A URL links to the custom font users are going to "
+               "generate PDFs in. (You can upload the font on Studio)"),


When we use Studio uploaded files in xblocks, we usually only support the "/static/" style URLs, because it's the only thing that works reliably across export/imports.

Please change the help text to:

Studio static URL to a custom font file to be used for PDF report. Example: "/static/myfont.ttf". Leave empty to use default fonts. You can upload custom TTF font files from the Content - Files & Uploads page.

You can use this method to convert "/static/..." URLs to absolute urls. It's ugly, but as far as I know there is no cleaner way and we are using this method in several of our xblocks.

mtyaka · 2018-07-26T05:15:41Z

eoc_journal/eoc_journal.py

+        course_name = self._get_course_name()
+
+        report_header_name = self.display_name or course_name
+        title = _('{course_name} Report'.format(


Nit: can we just use report_header_name for the title as well?

mtyaka · 2018-07-26T05:17:11Z

eoc_journal/pdf_generator.py

+    font_name_italic = tt2ps(font_name, 0, 1)
+    font_name_bold_italic = tt2ps(font_name, 1, 1)
+
+    stylesheet.add(ParagraphStyle(name='Normal',


Please change line breaks to comply with edx python styleguide: http://edx.readthedocs.io/projects/edx-developer-guide/en/latest/style_guides/python-guidelines.html#breaking-long-lines

mtyaka · 2018-07-26T05:20:16Z

eoc_journal/pdf_generator.py

+    reportlab.rl_config.warnOnMissingFontGlyphs = 0
+    pdfmetrics.registerFont(font)
+
+    font_name_bold = tt2ps(font_name, 1, 0)


Nice, I didn't know about the tt2ps function.

Me neither! It's saved us a lot of bad code.

mtyaka · 2018-07-26T05:26:43Z

eoc_journal/pdf_generator.py

+from reportlab.pdfbase.ttfonts import TTFont
+
+
+def _add_url_scheme(url):


I think we'll be able to remove this method since we only want to support Studio /static/* urls (and can therefore use _expand_static_url helper instead).

iamjazzar · 2018-07-28T10:29:19Z

@mtyaka, this is ready for another pass.

mtyaka

Thanks for the updates @AhmedAljazzar!

The PDF report currently errors for me in the LMS if I try to use default fonts (see inline comments). I think the reason that the tests don't catch this is due to a difference in runtimes - the test runtime does not provide the replace_urls function.

mtyaka · 2018-07-29T04:23:15Z

eoc_journal/eoc_journal.py

@@ -85,6 +90,13 @@ class EOCJournalXBlock(StudioEditableXBlockMixin, XBlock):
        list_values_provider=provide_pb_answer_list,
    )

+    pdf_title = String(
+        display_name=_("PDF Title"),
+        help=_("Title to of the PDF report. Leave blank to use the course title."),


Typo/leftover: "Title to of..." -> "Title of..."

mtyaka · 2018-07-29T04:28:05Z

eoc_journal/eoc_journal.py

        pdf_buffer = BytesIO()
-        document = SimpleDocTemplate(pdf_buffer, pagesize=pagesizes.letter, title=_("Report"))
+        course_name = self._get_course_name()


Nit: can we remove this line and change the line below to report_header_name = self.pdf_title or self._get_course_name() to avoid fetching the course when pdf_title is set.

mtyaka · 2018-07-29T04:32:23Z

tests/integration/test_eoc_journal.py

+        default_title = 'Testing Ways'
+        custom_title = 'My Custom Report Title'
+
+        # The default title is 'Course Journal'.


This comment is outdated, right? The default title is the course name now.

mtyaka · 2018-07-29T04:35:43Z

tests/integration/test_eoc_journal.py

+
+        # Patch _get_course_name method.
+        mock_course = Mock()
+        mock_course.return_value = 'Testing Ways'


Instead of hardcoding the value here, can you move it up into a module variable (similar to how default_answers_data and other mocked values are defined at the module top level).

mtyaka · 2018-07-29T09:37:42Z

eoc_journal/eoc_journal.py

@@ -85,6 +90,13 @@ class EOCJournalXBlock(StudioEditableXBlockMixin, XBlock):
        list_values_provider=provide_pb_answer_list,
    )

+    pdf_title = String(
+        display_name=_("PDF Title"),


I would rename this to "PDF Report Title" for consistency with the other options related to the PDF report.

mtyaka · 2018-07-29T09:47:44Z

eoc_journal/eoc_journal.py

@@ -175,11 +199,15 @@ def serve_pdf(self, request, _suffix):
        """
        Builds and serves a PDF document containing user's freeform answers.
        """
-        styles = getSampleStyleSheet()
+        font_path = self._expand_static_url(self.custom_font) if self.custom_font else None
+        styles = get_style_sheet(font_url=self._make_url_absolute(font_path))


This line currently fails if custom_font is not set, since _make_ur_absolute(font_path) returns a truthy value even if font_path is None.

If I try to generate a PDF report using default fonts, I get a TTFError: Not a TrueType font: version=0x0A0A0A0A exception.

I also have to catch this to avoid a 500 errors just in case the user entered an invalid path.

mtyaka · 2018-08-01T02:54:08Z

Thanks you @AhmedAljazzar, this is working perfectly now 👍

I tested this: I added a new EOC xblock to my course and answered some freeform answers from the same course. I verified that the PDF report contains my answers separated by horizontal lines; that by default my PDF's title equals the course name but I can provide a custom title; and that I can provide a custom font file to be used instead of the default font.
I read through the code
I checked for accessibility issues
Includes documentation (field help text)

iamjazzar · 2018-08-01T20:35:02Z

Thanks @mtyaka for revewing! :D

iamjazzar added 2 commits July 21, 2018 19:07

Horizantal line between questions

b6f987e

Default PDF header to Course Title

6cd863e

iamjazzar requested a review from mtyaka July 25, 2018 13:11

iamjazzar force-pushed the jazzar/custom-entry branch 2 times, most recently from 5d42fd8 to fd4bd27 Compare July 25, 2018 13:36

mtyaka requested changes Jul 26, 2018

View reviewed changes

iamjazzar force-pushed the jazzar/custom-entry branch 19 times, most recently from 22ac6de to b3d712d Compare July 28, 2018 10:25

iamjazzar force-pushed the jazzar/custom-entry branch 4 times, most recently from 040febe to 7245697 Compare July 28, 2018 11:33

iamjazzar force-pushed the jazzar/custom-entry branch from 7245697 to 9d006b0 Compare July 28, 2018 11:36

mtyaka requested changes Jul 29, 2018

View reviewed changes

iamjazzar force-pushed the jazzar/custom-entry branch 2 times, most recently from 76c4267 to 7ccd6ff Compare July 31, 2018 18:04

Use custom fonts in generated PDF reports

b8762f8

iamjazzar force-pushed the jazzar/custom-entry branch from 7ccd6ff to b8762f8 Compare July 31, 2018 18:09

mtyaka approved these changes Aug 1, 2018

View reviewed changes

mtyaka merged commit 58def61 into master Aug 1, 2018

mtyaka deleted the jazzar/custom-entry branch August 1, 2018 02:55

mtyaka mentioned this pull request Aug 1, 2018

Update xblock-eoc-journal. edx/edx-solutions-edx-platform#1126

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancements on the freeform-answers report #17

Enhancements on the freeform-answers report #17

iamjazzar commented Jul 25, 2018 •

edited

mtyaka left a comment

mtyaka Jul 26, 2018

mtyaka Jul 26, 2018

mtyaka Jul 26, 2018

iamjazzar Jul 27, 2018

mtyaka Jul 26, 2018

mtyaka Jul 26, 2018

iamjazzar Jul 27, 2018

mtyaka Jul 26, 2018

iamjazzar commented Jul 28, 2018

mtyaka left a comment

mtyaka Jul 29, 2018

mtyaka Jul 29, 2018

mtyaka Jul 29, 2018

mtyaka Jul 29, 2018

mtyaka Jul 29, 2018

mtyaka Jul 29, 2018

iamjazzar Jul 30, 2018

mtyaka commented Aug 1, 2018

iamjazzar commented Aug 1, 2018

		from reportlab.pdfbase.ttfonts import TTFont


		def _add_url_scheme(url):

Enhancements on the freeform-answers report #17

Enhancements on the freeform-answers report #17

Conversation

iamjazzar commented Jul 25, 2018 • edited

Description

Tests

Screenshot

mtyaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamjazzar commented Jul 28, 2018

mtyaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtyaka commented Aug 1, 2018

iamjazzar commented Aug 1, 2018

iamjazzar commented Jul 25, 2018 •

edited