Prevent moving scripts between unit groups when they have resources or vocabulary #38702

bethanyaconnor · 2021-01-22T20:35:11Z

A step in PLAT-505. This PR prevent adding or removing a script from a unit group if any lessons in that script have resources or vocabulary. This PR does not handle standalone scripts as I was having trouble getting that to work.

I'm definitely open to ideas on how to do this better. I originally wanted to add a model validation but I struggled to figure out where.

Lessons do not have a direct connection to course version and their course version will change only when their script's course version does, so that's not the right place for this check.
A script's course version could change without ever directly changing the script (if they're part of a unit group) so that won't catch all cases
Not all scripts are in unit groups so a validation on the unit group won't catch all cases
A script could change course version without the course version object being modified (if they're part of a unit group) so that won't catch all cases

So I've gone for the strategy of "closing the gaps". This unfortunately means that in places we'll have to bake the logic of how these relationships work into the code more than I'd like.

Background

Resources and vocabulary belong to a course version and it is expected that they belong to the same course version as any lesson they're in. This becomes an issue if a script's course version changes as the resources and vocabulary in lessons in that script will still belong to the previous course version.

Links

spec
jira

Testing story

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

…dding a script with resources or vocabulary to a unit group

davidsbailey

Nice progress Bethany! The new tests look good too.

davidsbailey · 2021-02-01T23:08:42Z

dashboard/db/schema.rb

@@ -466,7 +466,7 @@
    t.index ["name", "version"], name: "index_foorm_forms_on_name_and_version", unique: true
  end

-  create_table "foorm_libraries", options: "ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci", force: :cascade do |t|
+  create_table "foorm_libraries", options: "ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci", force: :cascade do |t|


this line looks like it should be reverted

Oops -- this file shouldn't be committed here at all 🤦‍♀️

davidsbailey · 2021-02-02T00:13:38Z

dashboard/app/models/unit_group.rb

-      script = Script.find_by_name!(script_name)
+    if scripts_to_delete.any?(&:prevent_course_version_change?)
+      raise 'Cannot remove scripts that have resources or vocabulary'
+    end


I am trying to wrap my head around what we will do in the case where a curriculum writer decides that they do not want a unit in their unit group any more. perhaps the curriculum writer will have to keep the script in the unit group until they are ready to destroy it entirely?

I don't necessarily see a better way, and I'd rather have this protection than not. So I am ok with waiting for someone to run into this before dealing with it.

If it doesn't have resources or vocabulary, it should be possible to remove it still. If it does, the options are going to be to go through each lesson and remove these objects or wait until they want to destroy it entirely I guess.

I will admit the error handling is not pretty on this but I'd be surprised if this is an error path hit commonly.

Hamms · 2021-02-02T21:04:36Z

Almost feels like resources and vocabularies should be primarily associated with a script (or with a course version? Whichever model is the closest to the source of truth for versioning), and then lessons should do something like reference some subset of that content for the script that they're in, rather than themselves being directly associated with resources and vocab.

Hamms · 2021-02-02T21:06:35Z

dashboard/app/models/unit_group.rb


-    new_scripts.each_with_index do |script_name, index|
-      script = Script.find_by_name!(script_name)
+    if scripts_to_delete.any?(&:prevent_course_version_change?)


is there a reason we want this logic to live here, rather than in, say, a before_destroy hook on Script?

The variables could be better named but this isn't actually destroying a script, just removing it from the unit group.

ahhhhh, right. Yeah, something like scripts_to_remove would be clearer

Agreed -- I'll make that change

bethanyaconnor · 2021-02-02T21:26:30Z

Vocabularies and resources are directly associated with a course version, which is how we do that versioning you're referencing. I'm not sure I understand what you're suggesting about referencing a subset? I'm very open to adjusting how we maintain these associations if we can do it without breaking things so ideas are very welcome :)

Hamms · 2021-02-02T21:32:07Z

Oh, nice! We're halfway to my suggestion already, then.

So right now, resources are unique per (course_version, key) combination. What if instead of lessons having a direct has_and_belongs_to_many relationship with resource, they just had a list of resource keys, and we find the specific resource for the lesson by getting the appropriate course version from the lesson's script.

Hamms

to clarify: my suggestions for an alternative approach are suggestions for future work, not something I think we need to do in this PR.

This change LGTM for what it is! Love the tests, too; very clear.

davidsbailey · 2021-02-02T22:44:04Z

Thank you for suggesting this, Elijah! I think there have been a bunch of times recently where the topic of "decoupling vs deduplication" have come up, and so far we (our team, the org, etc) have tended to side with deduplication. is this a case where the approach you're suggesting has a benefit that you'd describe as "decoupling", or is there another motivation?

Hamms · 2021-02-02T23:15:41Z

I don't know that I'd describe it as either decoupling or deduplication. Rather, this is about recognizing that the underlying implementation does not align with our intended usage scenarios. Or at least it doesn't align as well as it could. Right now we're addressing this by adding some situational logic to make sure that we avoid one scenario in which the misalignment could cause an issue. I'm suggesting that there is an underlying implementation which could be better aligned, and would render this special logic unnecessary.

bethanyaconnor added 6 commits January 15, 2021 12:55

Prevent changing course version in certain conditions

89ed364

Merge branch 'staging' into prevent-course-version-change-with-resources

59b26ea

Add protection for vocabularies as well

88491e9

Throw an exception when hitting the error case and also don't allow a…

fd1d8ab

…dding a script with resources or vocabulary to a unit group

Add tests

09a243a

Merge branch 'staging' into prevent-course-version-change-with-resources

67230d1

bethanyaconnor changed the title ~~Prevent course version change with resources~~ Prevent moving scripts between unit groups when they have resources or vocabulary Jan 29, 2021

bethanyaconnor marked this pull request as ready for review January 29, 2021 22:12

Reduce lookups

c0e5289

bethanyaconnor requested review from davidsbailey and Hamms February 1, 2021 19:22

davidsbailey approved these changes Feb 2, 2021

View reviewed changes

Hamms reviewed Feb 2, 2021

View reviewed changes

Hamms approved these changes Feb 2, 2021

View reviewed changes

bethanyaconnor added 3 commits February 2, 2021 15:30

Merge branch 'staging' into prevent-course-version-change-with-resources

b1ce6a6

Revert accidentally committed file

1448974

Rename a variable to make it clearer

0f30670

bethanyaconnor merged commit 0c18fb1 into staging Feb 3, 2021

bethanyaconnor deleted the prevent-course-version-change-with-resources branch February 3, 2021 22:53

bethanyaconnor mentioned this pull request Mar 9, 2021

Create ScriptsResource and UnitGroupsResource to eventually replace teacher resources #39367

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent moving scripts between unit groups when they have resources or vocabulary #38702

Prevent moving scripts between unit groups when they have resources or vocabulary #38702

bethanyaconnor commented Jan 22, 2021 •

edited

davidsbailey left a comment

davidsbailey Feb 1, 2021

bethanyaconnor Feb 2, 2021

davidsbailey Feb 2, 2021 •

edited

bethanyaconnor Feb 2, 2021

bethanyaconnor Feb 2, 2021

Hamms commented Feb 2, 2021

Hamms Feb 2, 2021

bethanyaconnor Feb 2, 2021

Hamms Feb 2, 2021

bethanyaconnor Feb 2, 2021

bethanyaconnor commented Feb 2, 2021

Hamms commented Feb 2, 2021

Hamms left a comment

davidsbailey commented Feb 2, 2021

Hamms commented Feb 2, 2021

Prevent moving scripts between unit groups when they have resources or vocabulary #38702

Prevent moving scripts between unit groups when they have resources or vocabulary #38702

Conversation

bethanyaconnor commented Jan 22, 2021 • edited

Background

Links

Testing story

Reviewer Checklist:

davidsbailey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsbailey Feb 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hamms commented Feb 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bethanyaconnor commented Feb 2, 2021

Hamms commented Feb 2, 2021

Hamms left a comment

Choose a reason for hiding this comment

davidsbailey commented Feb 2, 2021

Hamms commented Feb 2, 2021

bethanyaconnor commented Jan 22, 2021 •

edited

davidsbailey Feb 2, 2021 •

edited