Add DataDoc show page by megcrenshaw · Pull Request #48084 · code-dot-org/code-dot-org

megcrenshaw · 2022-09-14T14:39:58Z

This work is part of https://codedotorg.atlassian.net/browse/PLAT-1279 –– Curriculum Writers can add new Data Docs.

This PR allows you to see the data doc after you've created it. To see functionality, go to http://localhost-studio.code.org:3000/data_docs/new. It should make you add Levelbuilder permissions to your local account before you can view the page, and after putting in info, the site redirects to the newly-created data doc.

See functionality: https://watch.screencastify.com/v/kVoYeC6J8jnxu5iugfNK

Links

Spec
Engineering Plan

Testing story

I added unit tests for these. In a separate PR, will add a UI test, ensuring a user can create and then see the data doc.

Deployment strategy

Follow-up work

Add UI test, and move asset out of curriculum builder (tracked here https://codedotorg.atlassian.net/browse/PLAT-1956)

Add UI test

Privacy

Security

Caching

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

megcrenshaw · 2022-09-14T14:44:49Z

          <div style={styles.container}>
            <div style={{marginBottom: 5}}>Markdown:</div>
            <MarkdownEnabledTextarea
+              name={this.props.name}


This attribute gets passed to the textarea field, which triggers the HTML5 form to send its contents in the post request

megcrenshaw · 2022-09-14T17:29:43Z

    <a href="https://creativecommons.org/">
-      <img src="https://curriculum.code.org/static/img/creativeCommons-by-nc-sa.png" />
+      <img
+        src="https://curriculum.code.org/static/img/creativeCommons-by-nc-sa.png"


Since we're deprecating this URL, this asset will need to be moved, right? If so, I'll create a ticket for it

yes, good catch!

Great –– tracked here https://codedotorg.atlassian.net/browse/PLAT-1956, and mentioned in follow-up work

davidsbailey

awesome progress! I love the sizing of these PRs 👍

davidsbailey · 2022-09-14T17:41:29Z

    'courses/edit': './src/sites/studio/pages/courses/edit.js',
    'courses/new': './src/sites/studio/pages/courses/new.js',
    'data_docs/new': './src/sites/studio/pages/data_docs/new.js',
+    'data_docs/show': './src/sites/studio/pages/data_docs/show.js', // TODO [meg]: move to codeStudioEntries once launched


I am curious, why not put it there now?

Sounds good –– will move this

davidsbailey · 2022-09-14T17:47:53Z

    <a href="https://creativecommons.org/">
-      <img src="https://curriculum.code.org/static/img/creativeCommons-by-nc-sa.png" />
+      <img
+        src="https://curriculum.code.org/static/img/creativeCommons-by-nc-sa.png"


yes, good catch!

davidsbailey · 2022-09-14T17:49:22Z

+  const {dataDocName, dataDocContent} = props;
+
+  return (
+    <>


TIL about fragments! 💡

davidsbailey · 2022-09-14T17:56:03Z

@@ -1,5 +1,5 @@
 class DataDocsController < ApplicationController
-  before_action :require_levelbuilder_mode_or_test_env
+  before_action :require_levelbuilder_mode_or_test_env # TODO [meg]: add except: [:show] to launch


waiting is fine, but also this seems safe to do now -- as long as we are not linking to this page yet, the risk of a legit user ending up on this page and then becoming confused seems very low.

Great, sounds good –– will change permissions for the show page!

davidsbailey · 2022-09-14T18:17:29Z

@@ -0,0 +1,2 @@
+%script{src: webpack_asset_path('js/data_docs/show.js'), data: {dataDocName: @data_doc_name.to_json, dataDocContent: @data_doc_content.to_json}}


I see that this use of to_json works today, but is a little concerning because technically only things wrapped in [] and {} are valid json, and we have run into problems like this before when we try to upgrade. my recommendation would be to wrap these two fields in a single object like data: {dataDocs: {...}.to_json}. you could also just pass them as plain strings without to_json or getScriptData.

Oo, good to know! I ran into an error without the to_json but didn't investigate further ... will look into changing this

davidsbailey · 2022-09-14T18:25:56Z

+    sign_in @levelbuilder
+    new_key = "doc_key"
+    get :create, params: {key: new_key}
+    assert_match %r{/data_docs/#{new_key}$}, @response.headers['Location']


great test! one nit: rails provides a... railsier way to do this assertion:

code-dot-org/dashboard/test/controllers/followers_controller_test.rb

Line 31 in f55ce24

assert_redirected_to controller: 'sections', action: 'show', id: @picture_section.code

that's what I'd recommend since it seems less fragile. if that seems too abstract, this could be a possible compromise:

code-dot-org/dashboard/test/controllers/projects_controller_test.rb

Line 360 in cee6220

assert_redirected_to '/projects/public'

Oh, that's cool! Will do that 👍

davidsbailey · 2022-09-14T18:27:33Z

  test_user_gets_response_for :create, params: -> {@test_params}, user: :levelbuilder, response: :success
+
+  # TODO [meg] : all should be able to see `show` page, once launched
+  # for now, only levelbuilder can see it


again, this strategy is perfectly valid, but seems like unnecessary work to take these extra steps to protect these pages

Sounds good –– will move these

davidsbailey

looks great!

davidsbailey · 2022-09-14T20:47:30Z

+
+  # GET /data_docs/:key
+  def show
+    @data_doc = DataDoc.find_by(key: params[:key])


I am curious, do you get this for free due to the combination of load_and_authorize_resource her and param: :key in routes.rb ? I would suggest removing this statement and seeing if everything still works (CanCan should define @data_doc for you). if not... no need to investigate too much, sometimes CanCan doesn't quite work as expected when the param is not id.

Just tried it: @data_doc isn't defined ... also, in the create method, I originally tried

if @data_doc.save redirect_to @data_doc

and got the same problem (it tried to use the id in the URL, which I was surprised by given the param: :key in routes)

Meg Crenshaw added 14 commits September 13, 2022 15:08

Add DataDocView component

13e09d9

Pass in props from show.js page

05d8422

Pass in data from haml page

1875783

Add show action in controller with :key param

d36be08

Add show route with :key param

98cc9a0

Add show url

06c4bce

Add frontend tests

76bf3c6

Add cancan tests for show action

742e6cf

Test for data doc not found

5e3c7cc

Add name attribute from TextareaWithMarkdownPreview component

edf22fe

Add alt text to licensing

7c12720

Redirect to show page after save

70b5bf8

Modify styling of view page

20f1f38

Add test for redirect

4a7032d

megcrenshaw commented Sep 14, 2022

View reviewed changes

megcrenshaw requested a review from a team September 14, 2022 14:45

megcrenshaw commented Sep 14, 2022

View reviewed changes

davidsbailey reviewed Sep 14, 2022

View reviewed changes

Meg Crenshaw added 4 commits September 14, 2022 15:39

Make data passed in an object

304290f

Update data passed in and use less generic id

c31f048

Break up divs for two separate sections

176d03f

Allow anyone to see show page for data_doc

2db29dd

megcrenshaw requested a review from davidsbailey September 14, 2022 19:43

davidsbailey approved these changes Sep 14, 2022

View reviewed changes

megcrenshaw merged commit 61d72b7 into staging Sep 14, 2022

megcrenshaw deleted the add-show-page branch September 14, 2022 21:26

sanchitmalhotra126 mentioned this pull request Sep 16, 2022

Update create-release script to account for squash merge commits #48154

Merged

8 tasks

TurnerRiley mentioned this pull request Sep 22, 2022

Add edit DataDoc backend #48243

Merged

8 tasks

		@@ -0,0 +1,2 @@
		%script{src: webpack_asset_path('js/data_docs/show.js'), data: {dataDocName: @data_doc_name.to_json, dataDocContent: @data_doc_content.to_json}}

Uh oh!

Conversation

megcrenshaw commented Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Links

Testing story

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidsbailey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidsbailey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

megcrenshaw commented Sep 14, 2022 •

edited

Loading