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

make script level checkboxes on lesson edit page work in UI tests #37641

Merged
merged 6 commits into from Nov 29, 2020

Conversation

davidsbailey
Copy link
Member

@davidsbailey davidsbailey commented Nov 3, 2020

I decided to do this because this line looked like a problem waiting to happen:

if Rails.application.config.levelbuilder_mode
we shouldn't be checking levelbuilder mode explicitly, because we rely on being able to access levelbuilder routes in the test environment.

What this PR does:

  1. makes the script level summarize route include editable fields whenever it is being summarized "for edit", even if that is in a test environment like drone or the test machine where levelbuilder mode is not enabled
  2. adds a UI test case for editing script level properties in the lesson editor
  3. rename summarize_for_edit to summarize_for_lesson_edit for activity, activity section and script level

Future work

Ideally we would extract summarize_for_script_edit methods for lesson and script level, however since that extraction looked messy and we won't have script levels on the script edit page after PLAT-460 anyway, it seemed better to stop short of doing that work.

Testing story

  • new UI test case for editing script level properties in the lesson editor
  • manually verified that saving the beta script editor preserves the assessment flag on script levels

Reviewer 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

@davidsbailey davidsbailey changed the title Summarize script level for edit so checkboxes work in UI tests make script level checkboxes work in UI tests on lesson edit page Nov 3, 2020
@davidsbailey davidsbailey changed the title make script level checkboxes work in UI tests on lesson edit page make script level checkboxes on lesson edit page work in UI tests Nov 3, 2020
@@ -223,7 +223,7 @@ def summarize(include_bonus_levels = false)
title: localized_title,
lesson_group_display_name: lesson_group&.localized_display_name,
lockable: !!lockable,
levels: cached_levels.map {|l| l.summarize(false)},
levels: cached_levels.map {|sl| sl.summarize(false, for_edit: for_edit)},
Copy link
Member Author

Choose a reason for hiding this comment

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

note: we should no longer have to pass in for_edit here after PLAT-460 is done.

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.

This looks good to me.

Is there something special about this call to Rails.application.config.levelbuilder_mode because there are other places like in the update_text method for script.rb where I have also seen it called.

@davidsbailey
Copy link
Member Author

Is there something special about this call to Rails.application.config.levelbuilder_mode because there are other places like in the update_text method for script.rb where I have also seen it called.

Good question :-) Yes. Rails.application.config.levelbuilder_mode is still the right check before writing to the filesystem (.script files, .script_json, scripts.en.yml, etc). We disallow those on the test machine because writing files could mess up the deploy pipeline on the test machine. but if we want most other application functionality to be enabled in the test environment so that we can cover it in UI tests.

Heads up @molly-moen in case there are any places you need to use Rails.application.config.levelbuilder_mode instead of require_levelbuilder_mode_or_test_env to keep from writing files in the test environment for the Foorm stuff.

@davidsbailey davidsbailey merged commit e95cb61 into staging Nov 29, 2020
@davidsbailey davidsbailey deleted the summarize-sl-for-edit branch November 29, 2020 15:28
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