Can create new courses #15148

Bjvanminnen · 2017-05-17T00:16:02Z

This PR makes it so that we can create new courses (by going to /courses/new on LB). It also enforces some validation on the name, which is stricter than the name for PLC courses (in part because we will need to serialize these in ways that wont happen with PLC).

joshlory

Sorry for all the comments! Feel free to ignore the style comments, but we should add authorization to the CoursesController#create action.

joshlory · 2017-05-17T16:25:25Z

dashboard/app/controllers/courses_controller.rb

+      course_name = params[:course_name].tr('-', '_').titleize
+      course = Course.find_by_name!(course_name)
+      # only support this alternative course name for plc courses
+      raise ActiveRecord::RecordNotFound unless course.plc_course


I don't think this is necessary — find_by_name! should raise if the course isn't found.

i believe you are right

find_by_name! raises if the course is not found, but we also want to raise if we found a course, but it is a regular course rather than a plc_course

Good point. Could change find_by_name! -> find_by_name for clarity.

joshlory · 2017-05-17T16:27:09Z

dashboard/app/controllers/courses_controller.rb

+
+  def new
+    render 'new'
+  end


Nit: We should standardize on whether we want to include these boilerplate action definitions if they are only rendering the default view. I know you prefer explicit, but Rails is CoC by default 😛.

joshlory · 2017-05-17T16:29:58Z

dashboard/app/controllers/courses_controller.rb

+  end
+
+  def create
+    course = Course.new(name: params.require(:course).require(:name))


Worth pulling this out to a private course_params method, and re-using in the show action? For example: ScriptsController#script_params.

They use two different sets of params? show takes params[:course_name], where as here we have params[:course][:name]

joshlory · 2017-05-17T16:31:26Z

dashboard/test/models/course_test.rb

+    end
+  end
+
+  test "should allow uppercase letters if it is a plc course" do


Are we tracking the debt to unify & redirect professional learning courses that don't match the format?

It's somewhat less important for these to be unified, given that PLC courses aren't being serialized to a file based on their name (unlike regular courses in the near future).

joshlory · 2017-05-17T16:32:46Z

dashboard/app/models/course.rb

+
+  # As we read and write to files with the course name, to prevent directory
+  # traversal (for security reasons), we do not allow the name to start with a
+  # tilde or dot or contain a slash.


Same as in Script, this is redundant with the above name validation. If we really want defense-in-depth maybe we should pull this validator out as a concern?

joshlory · 2017-05-17T16:37:15Z

dashboard/app/models/course.rb

+    end
+  end
+
+  validates_with CourseNameValidator


Why this format instead of validates :name (with unless: :plc_course)? It looks like there's common functionality in Script we could maybe combine into a single concern. If we want to start standardizing on this format for all names that appear in URLs...

Also for custom validators that are only used in a single, it's simpler to just define a method (example). These ActiveModel::Validator classes are typically used for validations shared across models, for example.

You're way is much better than mine. I will try that approach.

joshlory · 2017-05-17T16:39:14Z

dashboard/app/controllers/courses_controller.rb

@@ -1,4 +1,6 @@
 class CoursesController < ApplicationController


Auth for modify actions?

aoby · 2017-05-17T16:42:00Z

dashboard/app/controllers/courses_controller.rb

+    if course.save
+      redirect_to action: :edit, course_name: course.name
+    else
+      render 'new', locals: {error_messages: course.errors.full_messages}


A common way of doing this is to set @course, and have the form in the view be form_for @course. That way any params are saved between attempts, and errors are already on the @course object and don't need to be passed separately.

I've been trying to avoid using instance variables in my views, as I find them much more difficult to follow than explicitly passing things in via locals. This might be a case where there's a good argument for doing it the way you say.

Or if you prefer it to be explicit, pass course as a local, do form_for course, and inspect course.errors

aoby · 2017-05-17T16:47:46Z

dashboard/app/models/course.rb

+    end
+  end
+
+  validates_with CourseNameValidator


Also for custom validators that are only used in a single, it's simpler to just define a method (example). These ActiveModel::Validator classes are typically used for validations shared across models, for example.

Bjvanminnen · 2017-05-17T20:59:41Z

@aoby @joshlory I believe I've addressed all feedback.

joshlory · 2017-05-17T22:13:33Z

dashboard/app/controllers/courses_controller.rb

+    else
+      render 'new', locals: {course: course}
+    end
+  end


Looks like this action is missing test coverage?

I'm not sure how interesting this method is to test. I might add a quick and dirty test in a future PR.

joshlory · 2017-05-17T22:14:27Z

dashboard/app/models/course.rb

+      unless: :plc_course,
+      with: /\A[a-z0-9\-]+\z/,
+      message: 'can only contain lowercase letters, numbers and dashes'
+    }


Would be good to have this de-duped against the name validation in Script.

Using a concern to do this may be easier to do than I thought. I will explore doing this in a follow on PR

Bjvanminnen added 2 commits May 16, 2017 16:14

action/view for creating a new course

ecf2115

can create a new course (with no scripts in it)

74a5e02

Bjvanminnen requested review from aoby and joshlory May 17, 2017 16:19

joshlory self-assigned this May 17, 2017

joshlory reviewed May 17, 2017

View reviewed changes

aoby reviewed May 17, 2017

View reviewed changes

address code review

2dee3b2

aoby approved these changes May 17, 2017

View reviewed changes

joshlory approved these changes May 17, 2017

View reviewed changes

Bjvanminnen merged commit 80f57e6 into staging May 17, 2017

Bjvanminnen deleted the newCourse branch May 17, 2017 22:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Can create new courses #15148

Can create new courses #15148

Bjvanminnen commented May 17, 2017

joshlory left a comment

joshlory May 17, 2017

Bjvanminnen May 17, 2017

Bjvanminnen May 17, 2017

joshlory May 17, 2017

joshlory May 17, 2017

joshlory May 17, 2017

Bjvanminnen May 17, 2017

joshlory May 17, 2017

Bjvanminnen May 17, 2017

joshlory May 17, 2017

joshlory May 17, 2017

aoby May 17, 2017

Bjvanminnen May 17, 2017

joshlory May 17, 2017

aoby May 17, 2017 •

edited

Bjvanminnen May 17, 2017

aoby May 17, 2017

aoby May 17, 2017

Bjvanminnen commented May 17, 2017

joshlory May 17, 2017

Bjvanminnen May 17, 2017

joshlory May 17, 2017

Bjvanminnen May 17, 2017

		@@ -1,4 +1,6 @@
		class CoursesController < ApplicationController

Can create new courses #15148

Can create new courses #15148

Conversation

Bjvanminnen commented May 17, 2017

joshlory 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

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

aoby May 17, 2017 • edited

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

Bjvanminnen commented May 17, 2017

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

aoby May 17, 2017 •

edited