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

Add data guide model #46453

Merged
merged 10 commits into from May 31, 2022
Merged

Add data guide model #46453

merged 10 commits into from May 31, 2022

Conversation

megcrenshaw
Copy link
Contributor

This begins the work for Req 1: Curriculum Writers can add new Data Docs. Adds the model for data guides––will add unit tests and validation for these next.

Links

Work Plan
PLAT-1279

Testing story

Deployment strategy

Follow-up work

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 megcrenshaw requested a review from a team as a code owner May 20, 2022 14:29
@megcrenshaw megcrenshaw requested a review from a team May 20, 2022 14:30
@davidsbailey
Copy link
Member

Top level question (sorry I missed this in the design doc review): does a Data Guide represent a Data Doc? If so I think we should get on the same page about what to call these, and try to be consistent. I know it is a bit of work to rename, so let's discuss before you do.

@megcrenshaw
Copy link
Contributor Author

Top level question (sorry I missed this in the design doc review): does a Data Guide represent a Data Doc? If so I think we should get on the same page about what to call these, and try to be consistent. I know it is a bit of work to rename, so let's discuss before you do.

Yes, a Data Guide is a Data Doc––I believe the idea was to keep consistency with a Reference Guide. But maybe that parallel is unnecessary. I think I was originally thinking it'd be Data Doc but someone else suggested Data Guide and I was good with that ;). What are you thinking, Dave?

@davidsbailey
Copy link
Member

Yes, a Data Guide is a Data Doc––I believe the idea was to keep consistency with a Reference Guide. But maybe that parallel is unnecessary. I think I was originally thinking it'd be Data Doc but someone else suggested Data Guide and I was good with that ;). What are you thinking, Dave?

My main thought here is that if we call it Data Guide in the code then we should also call it that in the edit UI and in our discussions in product meetings. Either one is fine with me as long as we're consistent. @tess323 do you have an opinion on whether to call these Data Guides vs Data Docs?

@tess323
Copy link

tess323 commented May 23, 2022

These have long been called Data Docs and that makes a lot of sense to me from the user perspective - these follow the Reference Guide data model, but are more akin to Documentation than the concept for 'Reference Guides'. My understanding was Data Guide was to be used as an internal/engineering term. If we are going to update the external-facing name I would want to get buy-in from Ken + Josh. Is there a compelling engineering reason to move to Data Guide - if so I am happy to take an action item to socialize this - if not I say we stick with Data Doc

@megcrenshaw
Copy link
Contributor Author

These have long been called Data Docs and that makes a lot of sense to me from the user perspective - these follow the Reference Guide data model, but are more akin to Documentation than the concept for 'Reference Guides'. My understanding was Data Guide was to be used as an internal/engineering term. If we are going to update the external-facing name I would want to get buy-in from Ken + Josh. Is there a compelling engineering reason to move to Data Guide - if so I am happy to take an action item to socialize this - if not I say we stick with Data Doc

No reason to switch to Data Guide––I'll update this to be Data Doc. Thanks, Dave, for the callout, and thanks, Tess, for the quick response!

@@ -457,6 +457,16 @@
t.index ["name"], name: "index_courses_on_name"
end

create_table "data_docs", options: "ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci", force: :cascade do |t|
Copy link
Member

Choose a reason for hiding this comment

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

unfortunately I am seeing some diffs when I apply this migration locally:

Dave-MBP:~/src/cdo/dashboard (staging)$ UTF8=1 RAILS_ENV=test bundle exec rake db:reset db:test:prepare

Dave-MBP:~/src/cdo/dashboard (staging)$ git checkout add-data-guide-model

Dave-MBP:~/src/cdo/dashboard (add-data-guide-model)$ RAILS_ENV=test bundle exec rake db:migrate                  
== 20220525181706 CreateDataDocs: migrating ===================================
-- create_table(:data_docs)
   -> 0.0277s
== 20220525181706 CreateDataDocs: migrated (0.0278s) ==========================

Dave-MBP:~/src/cdo/dashboard (add-data-guide-model *)$ git diff
diff --git a/dashboard/db/schema.rb b/dashboard/db/schema.rb
index 6f51582e9e4..5a43dc3764d 100644
--- a/dashboard/db/schema.rb
+++ b/dashboard/db/schema.rb
@@ -457,7 +457,7 @@ ActiveRecord::Schema.define(version: 2022_05_25_181706) do
     t.index ["name"], name: "index_courses_on_name"
   end
 
-  create_table "data_docs", options: "ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci", force: :cascade do |t|
+  create_table "data_docs", options: "ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci", force: :cascade do |t|
     t.string "key", null: false
     t.string "name"
     t.text "content"

what I would suggest is that you run the same steps that I have run here, and then commit the result to this PR. it should result in the "mb4" parts of this line going away.

apologies if you have heard contrary advice from others... I forget where we are at in the debate on this, so if you believe this will all get sorted out on staging then that could also be ok -- but it's worth noting that as long as one of your local databases has utf8mb4 as the default, you're likely to keep running into problems like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lol I thought I took care of that problem ... will try again. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

the above steps should get your local test database into a good state. if you are on board with what I'm suggesting, then you'll want to also get your local development db into a good state (step 11 in https://github.com/code-dot-org/code-dot-org/blob/2f70349a4548981b573392200b8960a92d75b6eb/SETUP.md#overview ).

then you would also need to run the migration backward and forward again on your development DB to get it to pick up the utf8 settings.

Copy link
Member

@davidsbailey davidsbailey left a comment

Choose a reason for hiding this comment

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

LGTM after addressing UTF8 issue.

@megcrenshaw megcrenshaw merged commit 49af967 into staging May 31, 2022
@megcrenshaw megcrenshaw deleted the add-data-guide-model branch May 31, 2022 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants