Use ProgrammingEnvironmentCategory in ProgrammingExpression #44735

bethanyaconnor · 2022-02-09T18:08:15Z

Put programming expressions in the new categories! This PR does a best attempt to match the expressions with a ProgrammingEnvironmentCategory and updates the programming expression edit page to use these. It also prevents deletion of any ProgrammingEnvironmentCategory that has a programming expression.

Note to reviewers: I'd suggest reviewing this in two parts. The code changes load a bit better by filtering out the json files. All the json file changes were in commit ca1ee8b so a quick look at that commit should be good for those changes.

Categories where some can be deleted and some can't:

Follow-up work

I realized that this will soon break our search for code docs on the lesson edit page. I'll fix that in the coming week or so.

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

programming_environment_categories and update two blocks based on their category on curriculumbuilder

…t and show pages

…ramming expressions

dmcavoy · 2022-02-09T20:26:17Z

I realized that this will soon break our search for code docs on the lesson edit page. I'll fix that in the coming week or so.

How soon will this break things? Why will it break things?

dmcavoy · 2022-02-09T20:27:56Z

apps/src/lib/levelbuilder/code-docs-editor/OrderableList.jsx

@@ -7,7 +7,8 @@ export default function OrderableList({
  list,
  setList,
  addButtonText,
-  renderItem
+  renderItem,
+  checkItemDeletionAllowed = item => true


Can you tell me about how this works?

Is this just setting a default if checkItemDeletionAllowed is not passed?

Yes it's just a default. All items can be deleted unless a specific function is passed in to check.

dmcavoy · 2022-02-09T20:30:37Z

apps/test/unit/lib/levelbuilder/code-docs-editor/OrderableListTest.jsx

@@ -39,9 +39,12 @@ describe('OrderableList', () => {
    const wrapper = shallow(<OrderableList {...defaultProps} />);
    wrapper
      .find('.fa-trash')
-      .at(1)
+      .at('1')


Should this at be changing to '1' too?

oops good catch. This is what I get for a too optimistic search/replace...

Gotta love JS allowing this to still work though

dmcavoy · 2022-02-09T20:31:55Z

apps/test/unit/lib/levelbuilder/code-docs-editor/OrderableListTest.jsx

+        checkItemDeletionAllowed={item => item.key === '3'}
+      />
+    );
+    expect(wrapper.find('.fa-trash').length).to.equal(1);


It looks like there should be 2 trash icons because there should be 3 items. Is that right?

Only the third item is deletable. I waffled on which was to frame the function, i.e. checkItemDeletionAllowed or preventItemDeletion. Do you think the second one is clearer?

Ohhhh I see. I think thats fine. I think I just didn't fully understand before.

dmcavoy · 2022-02-09T20:33:21Z

dashboard/app/controllers/programming_expressions_controller.rb

    programming_expression.palette_params = programming_expression_params[:parameters]
+    programming_environment_category = programming_expression.programming_environment.categories.find_by_key(programming_expression_params[:category_key])
+    programming_expression.programming_environment_category_id = programming_environment_category&.id
+    # TODO: get rid of this when we remove the category column


Is this the issue with search on lesson edit you called out?

Yes, it's related. The category column doesn't serve a function anymore so I was going to get rid of that column, but the lesson edit page allows searching by category.

dmcavoy · 2022-02-09T20:34:32Z

dashboard/app/models/programming_environment_category.rb

@@ -17,6 +17,7 @@
 #
 class ProgrammingEnvironmentCategory < ApplicationRecord
  belongs_to :programming_environment
+  has_many :programming_expressions, dependent: :restrict_with_error


bethanyaconnor · 2022-02-09T20:54:50Z

How soon will this break things? Why will it break things?

Right now, it's fine. But with category moving off of the ProgrammingExpression model, we'll no longer be able to search by category, which we currently support. This is likely not a huge deal and can be worked around, just something I noticed yesterday.

dmcavoy · 2022-02-09T21:01:52Z

dashboard/app/models/programming_expression.rb

+      if env_category
+        nil
+      else
+        environment_name == 'spritelab' ? expression_config['color'] : ProgrammingExpression.get_category_color(expression_config['category'])


what still falls back on the second case here?

There are a bunch of programming expressions that don't have a ProgrammingEnvironmentCateogry. I'm going to clean a bunch of those up over the next several days then hopefully we won't need this case anymore.

dmcavoy · 2022-02-09T21:04:48Z

dashboard/test/models/programming_expression_test.rb

-    exp = create :programming_expression, key: 'myExp', category: 'World', examples: 'myexamples', palette_params: 'some parameters', programming_environment_id: programming_environment.id
-    exp.color = ProgrammingExpression.get_category_color('World')
+    category = create :programming_environment_category, programming_environment: programming_environment, name: 'World', color: '#ABCDEF'
+    exp = create :programming_expression, key: 'myExp', category: 'World', examples: 'myexamples', palette_params: 'some parameters', programming_environment_id: programming_environment.id, programming_environment_category_id: category.id


Shoudl this still have category set separate from programming_environment_category?

Yeah probably. I'll add that back.

Oh sorry, misunderstood the comment. I think that's fine for now as we still seed and serialize the category column

dmcavoy

I only spot checked random of the json file changes but I think it looks good. Thanks for all your hard work on this

bethanyaconnor added 5 commits February 9, 2022 10:07

Seed and serialize category_key with programming expressions

6bba3a9

Add category_key to the expressions where their category exists in

ca1ee8b

programming_environment_categories and update two blocks based on their category on curriculumbuilder

Use ProgrammingEnvironmentCategory for the programming expression edi…

448e78e

…t and show pages

Don't allow deletion of ProgrammingEnvironmentCategory if it has prog…

1e92b8d

…ramming expressions

Add/update tests

bf1f9d8

bethanyaconnor force-pushed the programming-expression-categories branch from 575c7ee to bf1f9d8 Compare February 9, 2022 18:09

bethanyaconnor changed the title ~~Programming expression categories~~ Use ProgrammingEnvironmentCategory in ProgrammingExpression Feb 9, 2022

bethanyaconnor marked this pull request as ready for review February 9, 2022 20:21

bethanyaconnor requested review from a team and dmcavoy February 9, 2022 20:21

dmcavoy reviewed Feb 9, 2022

View reviewed changes

dmcavoy approved these changes Feb 9, 2022

View reviewed changes

Fix accidentally changed index

ce6d9f5

bethanyaconnor merged commit 6821145 into staging Feb 10, 2022

bethanyaconnor deleted the programming-expression-categories branch February 10, 2022 18:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ProgrammingEnvironmentCategory in ProgrammingExpression #44735

Use ProgrammingEnvironmentCategory in ProgrammingExpression #44735

bethanyaconnor commented Feb 9, 2022 •

edited

dmcavoy commented Feb 9, 2022

dmcavoy Feb 9, 2022

dmcavoy Feb 9, 2022

bethanyaconnor Feb 9, 2022

dmcavoy Feb 9, 2022

bethanyaconnor Feb 9, 2022

bethanyaconnor Feb 9, 2022

dmcavoy Feb 9, 2022

bethanyaconnor Feb 9, 2022

dmcavoy Feb 9, 2022

dmcavoy Feb 9, 2022

bethanyaconnor Feb 9, 2022

dmcavoy Feb 9, 2022

bethanyaconnor commented Feb 9, 2022

dmcavoy Feb 9, 2022

bethanyaconnor Feb 9, 2022

dmcavoy Feb 9, 2022

bethanyaconnor Feb 9, 2022

bethanyaconnor Feb 9, 2022

dmcavoy left a comment

Use ProgrammingEnvironmentCategory in ProgrammingExpression #44735

Use ProgrammingEnvironmentCategory in ProgrammingExpression #44735

Conversation

bethanyaconnor commented Feb 9, 2022 • edited

Follow-up work

PR Checklist:

dmcavoy commented Feb 9, 2022

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

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

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 9, 2022

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

Choose a reason for hiding this comment

dmcavoy left a comment

Choose a reason for hiding this comment

bethanyaconnor commented Feb 9, 2022 •

edited