Markdown Preprocessor for Lesson Plans #39310

Hamms · 2021-03-03T00:37:43Z

Adds the ability to render markdown syntax which requires database access.

Specifically, we add a "preprocess" step in the rendering process for certain markdown-enabled strings, which will replace certain serverside-only syntaxes with vanilla markdown equivalent. Currently used for Resource Links, will be extended to include Vocab Links in the near future, and could be further extended with other kinds of content in the distant future.

Links

Testing story

Added some pretty extensive unit tests. I'd be happy to add some ui tests as well if folks think they're necessary.

Follow-up work

Vocab Links
Better editor support. Right now, only Activity Sections have a 'add resource link' button and associated dialog; in a future PR, all other fields that support this functionality will be updated to have a similar ui. See refactor the TextareaWithImageUpload component to instead be MarkdownEnabledTextarea #39333

Caching

This functionality is heavily cached by the input strings. I don't imagine we'll ever be in a situation where we want to update Resource data on production outside the context of a deploy with it associated cache expiration, but feel free to let me know if I'm missing something.

PR Checklist:

Tests provide adequate coverage
Privacy and Security impacts have been assessed
Code is well-commented
New features are translatable or updates will not break translations
Relevant documentation has been added or updated
User impact is well-understood and desirable
Pull Request is labeled appropriately
Follow-up work items (including potential tech debt) are tracked and linked

…y as a whole

davidsbailey

This looks great, @Hamms ! Nice unit tests!

davidsbailey · 2021-03-04T20:34:45Z

dashboard/app/models/resource.rb

@@ -101,7 +102,7 @@ def generate_key_from_name
    # something simple like gsub (which can only do positive matches) we have
    # to do something manual.


I know this may be out of scope for this PR, but I think I am missing something about this comment here -- wouldn't something like the following work?

code-dot-org/lib/cdo/aws/cloud_formation.rb

Line 29 in 8ea35ce

STACK_NAME_INVALID_REGEX = /[^[[:alnum:]-]]/

code-dot-org/lib/cdo/aws/cloud_formation.rb

Line 47 in 8ea35ce

stack_name.gsub!(STACK_NAME_INVALID_REGEX, '-')

I'd like to have a single regex that's the source of truth. I don't want to have to do something like

VALID_KEY_CHAR_RE = /[a-z0-9\-\_]/ INVALID_KEY_CHAR_RE = /[^a-z0-9\-\_]/

davidsbailey · 2021-03-04T20:39:29Z

dashboard/lib/services/markdown_preprocessor.rb

+    # for relevant documentation
+    def self.process!(content, cache_options: nil)
+      return content unless content.present?
+      cache_key = "MarkdownPreprocessor/process/#{Digest::MD5.hexdigest(content)}"


I am curious, what is the motivation for hashing the cache key? It seems like it might(?) make things harder to debug, and I'm having trouble imagining inputs that would be much longer than an MD5 hexdigest:

[development] dashboard > Digest::MD5.hexdigest('[v computer-science]') => "ea78c2daad5e16fc4e4ef1a21fa98e61"

oh, or is it the entire block of text (overview, activity section description, etc)? if so, makes sense to hash!

yeah, it's the entire block

davidsbailey · 2021-03-04T21:09:53Z

dashboard/test/lib/services/markdown_preprocessor_test.rb

+    end
+  end
+
+  test 'regular method returns and does not modifiy' do


nit: "modify"

davidsbailey · 2021-03-04T21:15:57Z

dashboard/test/models/lesson_test.rb

+  test 'lesson edit summary does not preprocess markdown' do
+    lesson = create :lesson, lesson_group: create(:lesson_group)
+    Services::MarkdownPreprocessor.expects(:process).never
+    lesson.summarize_for_lesson_edit
+  end


better to expects(:process!) here, to prevent calls to both .process and .process!?

dmcavoy · 2021-03-04T22:00:21Z

apps/src/lib/levelbuilder/lesson-editor/FindResourceDialog.jsx

@@ -56,6 +56,12 @@ class FindResourceDialog extends Component {
            ))}
          </select>
        </label>
+        <p>


Will the "real" link show in the preview on the right side of the edit page?

Great question Dani! I don't think it will be practical to show this during live editing for April, so my thought would be to let the live preview just show the raw [r ...] syntax. One idea for the future would be to show these live previews with resolved links after each time the page is saved -- though it seems like we'd have to stop resolving the links as soon as the editor started changing the markdown, unless we want to send ajax requests to resolve them again.

I've got a couple ideas of ways we could do various kinds of fanciness to get something like what you're talking about Dave to work, but I agree that they're all too much to try to get done for April.

Sounds good! Should we track the follow up in Jira for Q2?

Yes, please!

…y the other, so by testing against that we actually verify both

…taxes Since those values are necessary to _actually_ uniquely identify resources and vocabularies. Depends on #39461; follow-up to #39447 and #39310

Hamms added 6 commits March 2, 2021 16:28

extract regex for individual key characters from the regex for the ke…

1556137

…y as a whole

add a basic 'markdown preprocessor' for lesson plans

326269d

add proof-of-concept for actually invoking the preprocessor

4455e94

add caching

966fcc7

connect the full set of fields we want to preprocess to the preprocessor

886e831

add editor-visible explanation for what to expect from resource links

20ed784

Hamms marked this pull request as ready for review March 4, 2021 00:48

Hamms requested a review from a team March 4, 2021 00:48

davidsbailey approved these changes Mar 4, 2021

View reviewed changes

dmcavoy reviewed Mar 4, 2021

View reviewed changes

Hamms added 3 commits March 4, 2021 15:10

typo fix

00d8adc

target expectation on the processbang method; that one gets invoked b…

9a55d32

…y the other, so by testing against that we actually verify both

Merge branch 'staging' into lesson-plan-markdown-preprocessor

d216882

Hamms merged commit d2e68f5 into staging Mar 8, 2021

Hamms deleted the lesson-plan-markdown-preprocessor branch March 8, 2021 17:20

Hamms mentioned this pull request Mar 11, 2021

Add vocabulary definitions functionality to the markdown preprocessor #39447

Merged

8 tasks

Hamms mentioned this pull request Mar 11, 2021

Add Course Offering and Version keys to the Markdown Preprocessor syntaxes #39463

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Markdown Preprocessor for Lesson Plans #39310

Markdown Preprocessor for Lesson Plans #39310

Hamms commented Mar 3, 2021 •

edited

davidsbailey left a comment

davidsbailey Mar 4, 2021

Hamms Mar 4, 2021 •

edited

davidsbailey Mar 4, 2021

davidsbailey Mar 4, 2021

Hamms Mar 4, 2021

davidsbailey Mar 4, 2021

davidsbailey Mar 4, 2021

dmcavoy Mar 4, 2021

davidsbailey Mar 4, 2021

Hamms Mar 4, 2021

dmcavoy Mar 5, 2021

davidsbailey Mar 5, 2021

		@@ -101,7 +102,7 @@ def generate_key_from_name
		# something simple like gsub (which can only do positive matches) we have
		# to do something manual.

Markdown Preprocessor for Lesson Plans #39310

Markdown Preprocessor for Lesson Plans #39310

Conversation

Hamms commented Mar 3, 2021 • edited

Links

Testing story

Follow-up work

Caching

PR Checklist:

davidsbailey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hamms Mar 4, 2021 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hamms commented Mar 3, 2021 •

edited

Hamms Mar 4, 2021 •

edited