Add a page to view all vocabulary in a course version #38760

bethanyaconnor · 2021-01-27T22:34:10Z

This PR adds:

A page to view all vocabulary associated with a course version, at the url /courses/:course_name/vocab/edit.
The ability to add new vocabulary from that page
The ability to edit existing vocabulary from that page
The ability to permanently destroy vocabulary from that page

Follow-up:

I didn't add the ability to filter vocabulary because the most vocabulary in a course version is 48, easily searchable from the browser. This might not be a sustainable solution for vocabulary and filtering will be needed for the All Resources page
I'd like to add a way to see (or edit!) the lessons a vocabulary is in. I didn't do that in this PR as it didn't feel like critical functionality but I'll do it as a follow-up if others think that would be useful

Other notes:

I wasn't sure which controller to put this in. The url suggests courses_controller but the fact that this page is really only about vocabulary suggests vocabularies_controller. I put this functionality in the latter for now, but it would be fairly easy to move
Along the same vein, I wasn't sure where to put the React component either so open to other suggestions!

Screenshot for http://localhost-studio.code.org:3000/courses/coursea-2021/vocab/edit

Deleting a vocabulary:

Editing a vocabulary

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

…ty to add new vocabulary and edit existing vocabulary

bethanyaconnor · 2021-01-29T16:58:27Z

Tagging @tess323 here too in case she has any thoughts about what I've got here and/or any follow-up steps

tess323 · 2021-01-29T17:32:52Z

This is awesome @bethanyaconnor! I agree in the short term search isn't necessary. If it would take less than a day (day and a half) to add lessons tagged with that vocab I would go ahead and do it! Anything longer we should chat about - my major question is will doing it here make it easier to do it on things like resources

dmcavoy

I think /lib/levelbuilder seems like the right place to put this since thats where all the other new editors are.

dmcavoy · 2021-01-29T21:37:05Z

apps/src/lib/levelbuilder/AllVocabulariesEditor.jsx

+            style={styles.addButton}
+            type="button"
+          >
+            <i className="fa fa-plus" style={{marginRight: 7}} /> Create New


nit: Should "Create New" be on the next line with "Vocabulary"?

hmm prettier is breaking it up...I'll just move the whole thing to the next line

dmcavoy · 2021-01-29T21:39:41Z

apps/test/unit/lib/levelbuilder/AllVocabulariesEditorTest.js

+    expect(wrapper.find('tr').length).to.equal(3);
+  });
+
+  it('can remove a vocabulary', () => {


maybe add a test for checking you can add vocabulary?

I was struggling on what to actually test for that path as the logic is mostly handled through another component. I'm adding a test that clicking the button actually brings up that component but not sure what else to add.

dmcavoy · 2021-01-29T21:40:12Z

dashboard/app/controllers/vocabularies_controller.rb

@@ -36,11 +36,25 @@ def update
    end
  end

+  # GET /courses/:course_name/vocab/edit


It makes sense to me to have this in this controller

dmcavoy · 2021-01-29T21:42:42Z

dashboard/test/controllers/vocabularies_controller_test.rb

@@ -43,4 +43,28 @@ class VocabulariesControllerTest < ActionController::TestCase
    vocabulary.reload
    assert_equal 'definition', vocabulary.definition
  end
+
+  test "can load edit page of unit group course version" do


nit: "can load vocab edit page..." (here and below).

dmcavoy

Bethany this looks awesome! Thanks for building on this page that can be a template for future pages of this kind! One request for another test and some nits but overall looks good to me

davidsbailey

Awesome progress @bethanyaconnor , this looks great!

I wasn't sure which controller to put this in. The url suggests courses_controller but the fact that this page is really only about vocabulary suggests vocabularies_controller. I put this functionality in the latter for now, but it would be fairly easy to move

vocabularies_controller sounds right to me!

davidsbailey · 2021-01-30T00:13:25Z

dashboard/app/controllers/vocabularies_controller.rb

+  def find_matching_course_version
+    matching_unit_group = UnitGroup.find_by_name(params[:course_name])
+    return matching_unit_group.course_version if matching_unit_group
+    matching_standalone_course = Script.find_by_name(params[:course_name])
+    return matching_standalone_course&.get_course_version if matching_standalone_course&.is_course
+    return nil
+  end


The above method looks like it might be some of the mess you were talking about as it relates to course version. I don't see a way around this in the short term. in the future, we want to move properties like "name" out of the Script and UnitGroup and into the CourseVersion. Then, this kind of operation should be easier to perform.

Does any of this seem a bit more sane knowing that "name" should eventually get moved into CourseVersion?

Yes that would be so much easier here! That's essentially what this function is looking for, except that we can't know which model holds the name until we look for it

Furthermore -- one of the reasons I was considering not having this logic in vocabularies_controller was because this helper function would need to be shared/duplicated when we write the All Resources page. I figured I could kick that can down the road and move it when it actually needs to be shared, but having a simpler way to query for a course would make this issue largely moot.

Good point -- this doesn't really belong in the vocabularies controller. when it comes time to build the All Resources page, I think it would make sense to move this into the CourseVersion model. I don't think we'll get to moving the name out of Script and UnitGroup into CourseVersion until "Q2 Tech Debt".

davidsbailey · 2021-01-30T00:16:36Z

dashboard/app/controllers/vocabularies_controller.rb

+    matching_unit_group = UnitGroup.find_by_name(params[:course_name])
+    return matching_unit_group.course_version if matching_unit_group
+    matching_standalone_course = Script.find_by_name(params[:course_name])
+    return matching_standalone_course&.get_course_version if matching_standalone_course&.is_course


on this line in particular, I think it might be better to do:

Suggested change

return matching_standalone_course&.get_course_version if matching_standalone_course&.is_course

return matching_standalone_course&.course_version if matching_standalone_course&.is_course

I think this will do the same thing as the code you have here, but it's a bit more straight forward because we are already checking that we're handling the case where this script is not in a unit group.

Add a page to view all vocabulary in a course version, with the abili…

4181588

…ty to add new vocabulary and edit existing vocabulary

bethanyaconnor force-pushed the all-vocab-edit-page branch from ac39bf0 to 4181588 Compare January 27, 2021 22:42

bethanyaconnor added 2 commits January 27, 2021 15:55

Add tests for controller actions

bf642d4

Add apps tests for AllVocabulariesEditor

da5ae89

Base automatically changed from add-edit-vocabulary to staging January 28, 2021 18:44

bethanyaconnor changed the title ~~Add a page to view all vocabulary in a course version, with the abili…~~ Add a page to view all vocabulary in a course version Jan 28, 2021

bethanyaconnor changed the base branch from staging to revert-38780-revert-38694-add-edit-vocabulary January 28, 2021 20:42

Base automatically changed from revert-38780-revert-38694-add-edit-vocabulary to staging January 28, 2021 23:39

Merge branch 'staging' into all-vocab-edit-page

49cd74d

bethanyaconnor marked this pull request as ready for review January 29, 2021 16:03

bethanyaconnor requested review from davidsbailey and dmcavoy January 29, 2021 16:57

dmcavoy reviewed Jan 29, 2021

View reviewed changes

dmcavoy approved these changes Jan 29, 2021

View reviewed changes

davidsbailey approved these changes Jan 30, 2021

View reviewed changes

bethanyaconnor added 2 commits February 1, 2021 11:08

PR feedback: spacing, test name changes, use course_version directly

eb7065b

Add a test for Create New Vocabulary button

b8662a3

bethanyaconnor merged commit 6a7500c into staging Feb 1, 2021

bethanyaconnor deleted the all-vocab-edit-page branch February 1, 2021 20:56

bethanyaconnor mentioned this pull request Feb 2, 2021

Edit vocabulary lessons in dialog #38839

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a page to view all vocabulary in a course version #38760

Add a page to view all vocabulary in a course version #38760

bethanyaconnor commented Jan 27, 2021 •

edited by davidsbailey

bethanyaconnor commented Jan 29, 2021

tess323 commented Jan 29, 2021 •

edited

dmcavoy left a comment

dmcavoy Jan 29, 2021

bethanyaconnor Feb 1, 2021

dmcavoy Jan 29, 2021

bethanyaconnor Feb 1, 2021

dmcavoy Jan 29, 2021

dmcavoy Jan 29, 2021

dmcavoy left a comment

davidsbailey left a comment

davidsbailey Jan 30, 2021

bethanyaconnor Feb 1, 2021

bethanyaconnor Feb 1, 2021

davidsbailey Feb 1, 2021

davidsbailey Jan 30, 2021

	return matching_standalone_course&.get_course_version if matching_standalone_course&.is_course
	return matching_standalone_course&.course_version if matching_standalone_course&.is_course

Add a page to view all vocabulary in a course version #38760

Add a page to view all vocabulary in a course version #38760

Conversation

bethanyaconnor commented Jan 27, 2021 • edited by davidsbailey

Links

Testing story

Reviewer Checklist:

bethanyaconnor commented Jan 29, 2021

tess323 commented Jan 29, 2021 • edited

dmcavoy left a 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

dmcavoy left a comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bethanyaconnor commented Jan 27, 2021 •

edited by davidsbailey

tess323 commented Jan 29, 2021 •

edited