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.

Sign up for GitHub

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

Jump to bottom

stop writing lesson descriptions to scripts.en.yml #46188

Merged
merged 13 commits into from
May 6, 2022
Merged

stop writing lesson descriptions to scripts.en.yml #46188

merged 13 commits into from
May 6, 2022

Conversation

davidsbailey
Copy link
Member

@davidsbailey davidsbailey commented May 5, 2022 •
edited

step 5a of PLAT-1528. This should have an immediate reduction in write contention on scripts.en.yml, because many of the edits made to this file are due to lesson descriptions being updated.

The real work is done in this commit: b0dc844

the rest of the PR removes a dead codepath which the script edit page previously used to import lesson descriptions from Curriculum Builder. this does not immediately reduce write contention, because we should already be skipping these writes here:

code-dot-org/dashboard/app/models/script.rb

Line 1339 in 6739e3b

if i18n == updated_i18n
however, it helps set us up for step 6 where we remove lesson descriptions from scripts.en.yml entirely.

Testing story

  • updated existing unit test coverage
  • spot-checked that we are not using description_student or description_teacher lesson translations anywhere else in our codebase

@davidsbailey davidsbailey changed the title remove lesson descriptions from lesson.i18n_hash stop writing lesson descriptions to scripts.en.yml May 5, 2022
@davidsbailey davidsbailey marked this pull request as ready for review May 5, 2022 21:39
@davidsbailey davidsbailey requested a review from a team May 5, 2022 21:39
davidsbailey
davidsbailey commented May 5, 2022
View reviewed changes
apps/src/lib/levelbuilder/unit-editor/UnitEditor.jsx
@@ -911,21 +904,6 @@ class UnitEditor extends React.Component {
this.setState({projectWidgetTypes})
}
/>
{!this.props.isMigrated && (
Copy link
Member Author

Choose a reason for hiding this comment

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

I probably should have started deleting from this point in the code, since everything on levelbuilder contingent upon !isMigrated is dead 🤷

davidsbailey
davidsbailey commented May 5, 2022
View reviewed changes
dashboard/app/models/script.rb
@@ -1635,40 +1622,27 @@ def summarize_i18n_for_copy(new_name, new_course_version)
data['lessons'] = {}
lessons.each do |lesson|
lesson_data = {
'key' => lesson.key,
Copy link
Member Author

Choose a reason for hiding this comment

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

@bethanyaconnor , was there any special reason you included the key when you copied lesson translations? It didn't exist for any scripts before 2021. these are the scripts I found them on:

ai-ethics-2021
coursea-2021
courseb-2021
coursec-2021
coursed-2021
coursee-2021
coursef-2021
csa6-2022
csa7-2022
csa8-2022
csd1-2021
csd1-2022
csd2-2021
csd2-2022
csd3-2022
csd4-preview-2021
csd5-2022
csd5-preview-2021
csd7-2022
csp1-2021
csp1-2022
csp10-2021
csp2-2021
csp2-2022
csp3-2021
csp3-2022
csp4-2021
csp4-2022
csp5-2021
csp5-2022
csp6-2021
csp6-2022
csp7-2021
csp7-2022
csp8-2021
csp9-2021
csp9-2022
express-2021
express-2022
hello-world-animals-2021
hello-world-emoji-2021
hello-world-food-2021
hello-world-retro-2021
k5-onlinepd-2021
k5-onlinepd-2022
kodea-pd-2021
poem-art-2021
pre-express-2021
pre-express-2022
self-paced-pl-csd2-2022
vpl-csd-2021
vpl-csd-2022
vpl-csp-2021
vpl-csp-2022

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like that line was added in https://github.com/code-dot-org/code-dot-org/pull/36020/files#diff-22769ab80ff34eaf4aadc1dc87e1b9bdad8b445cec233c9a5a1d2bcc5a7a7978, which was before my time on cplat, so I can't provide background, sorry

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, sorry to jump to conclusions Bethany. At the link you shared, it looks like a copy-paste error to me -- we were trying to populate the new lesson.key field from lesson.name, and it looks like we incorrectly also added a translation for the key at the same time, without a scenario where we'd need to ever show that translated value to the user. So I think this line is safe to remove.

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries -- I just didn't remember writing this :) I agree that it looks it can be removed. We never want to use translated unique identifiers anyway so it's better to not even give ourselves the option

@davidsbailey davidsbailey mentioned this pull request May 6, 2022
Merged
@dmcavoy
Copy link
Contributor

dmcavoy commented May 6, 2022 •
edited

I want to check my understanding. Looking at these lines:

code-dot-org/dashboard/app/models/lesson.rb

Lines 275 to 278 in b7fdd2a

description_student = get_localized_property('student_overview') || ''
description_student = render_codespan_only_markdown(description_student) unless script.is_migrated?
description_teacher = get_localized_property('overview') || ''
description_teacher = render_codespan_only_markdown(description_teacher) unless script.is_migrated?

It looks like we are only using the description_teacher and description_student if a lesson is in an unmigrated script. Are there any unmigrated scripts? Should we remove the lines which work for an unmigrated script here too?

@davidsbailey
Copy link
Member Author

It looks like we are only using the description_teacher and description_student if a lesson is in an unmigrated script. Are there any unmigrated scripts? Should we remove the lines which work for an unmigrated script here too?

Great question Dani! We have unmigrated scripts in production, but not in levelbuilder.

We did our migration by migrating all scripts on levelbuilder, and then propagating those changes to production via github and the seed process. because there are scripts in production which no longer exist on levelbuilder, those scripts in production were not migrated.

Once we do our first one-time cleanup of unused scripts in production, those unmigrated scripts will all be removed. then we'll be able to remove any code which affects only unmigrated scripts.

@davidsbailey
Copy link
Member Author

So, we shouldn't remove the lines you linked to because they affect scripts in production. but we can remove the lines I'm removing because they only get used on levelbuilder (to be verified by the reviewer, of course :-) )

dmcavoy
dmcavoy approved these changes May 6, 2022
View reviewed changes
Copy link
Contributor

@dmcavoy dmcavoy left a comment

Choose a reason for hiding this comment

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

Love to see these complicated code paths getting simpler!

@davidsbailey
Copy link
Member Author

Thank you for taking a close look, Dani!

@davidsbailey davidsbailey merged commit 0bb0218 into staging May 6, 2022
@davidsbailey davidsbailey deleted the lesson-description-yml branch May 6, 2022 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bethanyaconnor bethanyaconnor bethanyaconnor left review comments

@dmcavoy dmcavoy dmcavoy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@davidsbailey @dmcavoy @bethanyaconnor