Skip to content

move curriculum UI tests onto modular courses#66462

Merged
davidsbailey merged 13 commits into
stagingfrom
original-otherthings-course
Jun 24, 2025
Merged

move curriculum UI tests onto modular courses#66462
davidsbailey merged 13 commits into
stagingfrom
original-otherthings-course

Conversation

@davidsbailey

@davidsbailey davidsbailey commented Jun 11, 2025

Copy link
Copy Markdown
Member

starts https://codedotorg.atlassian.net/browse/TEACH-1970. also see:

The goal is to get more coverage of modularity functionality by moving existing unit test coverage to use modular courses. We anticipate that the most common failure mode for modular courses is for a link or user action to fall through to the original course rather than staying in the modular course.

This PR does the following:

  • changes some allthe* courses to have different original units, e.g. allthettsthings unit is still a part of allthettsthings course, but is now owned by a new original-allthettsthings-course
  • original courses have a published state of in_development, so that any anyone without levelbuilder permissions will immediately get a no_access warning on any script level page. For example: Chrome_teacher_tools_level_types_bubble_choice_output.html . Depends on Teach 1975/ensure in development units are not visible to signed out users  #66459.
  • allthemigratedthings is renamed to allthelessonplans because seeding will fail if you try to update the original_unit_group of a unit that has resources or vocab. I used clone_migrated_unit for this step, so it looks like a move but is really a copy and a deletion.

resolved UI test issues:

Testing story

Follow-up work

  • TEACH-2045 require modular course to have same participant audience as original course

@davidsbailey davidsbailey force-pushed the original-otherthings-course branch 2 times, most recently from 800a202 to 83e39b8 Compare June 12, 2025 17:23
@davidsbailey davidsbailey changed the base branch from staging to TEACH-1975/Ensure-in-development-units-are-not-visible-to-signed-out-users- June 17, 2025 22:06
@davidsbailey davidsbailey changed the base branch from TEACH-1975/Ensure-in-development-units-are-not-visible-to-signed-out-users- to staging June 17, 2025 22:06
@davidsbailey davidsbailey changed the base branch from staging to dave-merge-base June 17, 2025 22:12
@davidsbailey davidsbailey force-pushed the original-otherthings-course branch from 86e01ae to c84bdc0 Compare June 17, 2025 22:29
@davidsbailey davidsbailey force-pushed the original-otherthings-course branch from b080bba to d628a78 Compare June 19, 2025 18:59
@davidsbailey davidsbailey changed the base branch from dave-merge-base to staging June 19, 2025 19:00
@davidsbailey davidsbailey marked this pull request as ready for review June 23, 2025 17:24

@alex-m-brown alex-m-brown left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, just had a question. Should allthemigratedthings be completely removed if it's being replaced?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should the descriptions for allthelessonplans match allthemigrated things? Not sure if it really matters unless it's in an eyes test.

allthemigratedthings:
  title: All the Migrated Things
  description_short: ''
  description_student: Student overview of the unit.
  description_teacher: Teacher overview of the unit.
  version_title: '2020'

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

another great catch! it looks like these eyes tests will likely be affected:

  • dashboard/test/ui/features/teacher_tools/lesson_show.feature
  • dashboard/test/ui/features/teacher_tools/teacher_lesson_plan.feature

I think it's a good idea to have content here, so I've added back the descriptions but also made a couple of tweaks that will likely cause eyes diffs:

  • keeping the new name "All The Lesson Plans"
  • updating student and teacher descriptions to say "course" instead of "unit"

I also copied some more translations over to the unit, which I thought would have been copied over by clone_migrated_unit but were not. The only thing different from the original there is:

  • new name "All The Lesson Plans"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

heads up @drizco (Tuesday DOTD) that there will be some expected eyes diffs in these two tests:

  • dashboard/test/ui/features/teacher_tools/lesson_show.feature
  • dashboard/test/ui/features/teacher_tools/teacher_lesson_plan.feature

@davidsbailey

Copy link
Copy Markdown
Member Author

LGTM, just had a question. Should allthemigratedthings be completely removed if it's being replaced?

@alex-m-brown is your question just whether to remove the translations from courses/en.yml?

deletion of units is a slippery concept -- deleting the .script_json file won't cause the unit to be deleted from other environments (including production), so the translation strings could still get used even though the unit is gone, though that probably won't be a big problem for a test-only unit that was in beta.

deleting the .script_json file as i have done here does not leave things in a great state, but I'm not sure what alternative would be better. one idea would be that I keep the allthemigratedthings unit, and just remove all of its contents. thoughts?

@alex-m-brown

Copy link
Copy Markdown
Contributor

my question was both about removing the allthemigratedthings.course (and probably .script_json) file and removing the strings from courses/en.yml (and scripts/en.yml). I didn't see the files deleted in this PR. Like you said, it seems pretty low-risk either way since it's a test course.

@davidsbailey

Copy link
Copy Markdown
Member Author

Nice catch! I thought I had deleted some of this but it appears I did not.

Since deletions of units and unit groups do not get propagated to other environments, I've opted for unpublishing the course and removing the lesson content from the unit.

@davidsbailey davidsbailey merged commit e50a684 into staging Jun 24, 2025
6 checks passed
@davidsbailey davidsbailey deleted the original-otherthings-course branch June 24, 2025 03:57
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.

2 participants