Merge pull request #1244 from concord-consortium/185948944-fix-teache…

…r-class-api

fix: change teacher class API to accept Class ID instead of TeacherClass ID

rails/app/controllers/api/v1/teacher_classes_controller.rb

@@ -26,15 +26,17 @@ def sort
     # and we need the order preserved so we can update the position based on the id order
     ids.each do |id|
       begin
-        teacher_clazz = Portal::TeacherClazz.find(id)
+        clazz = Portal::Clazz.find(id)
       rescue ActiveRecord::RecordNotFound => e
-        return error("Invalid teacher class id: #{id}") if !teacher_clazz
+        return error("Invalid class id: #{id}")
       end
-      return error("You are not a teacher of class: #{id}") if !teacher_clazz.clazz.is_teacher?(user)
+      return error("You are not a teacher of class: #{id}") if !clazz.is_teacher?(user)
     end
 
     ids.each_with_index do |id,idx|
-      Portal::TeacherClazz.update(id, :position => (idx + 1))
+      teacher_clazz = Portal::TeacherClazz.where(:clazz_id => id, :teacher_id => user.portal_teacher.id).first
+      return error("TeacherClazz not found") if !teacher_clazz
+      teacher_clazz.update(:position => idx + 1)
     end
 
     render_ok
@@ -45,11 +47,10 @@ def copy
     return error(auth[:error]) if auth[:error]
     user = auth[:user]
 
-    class_ownership = verify_teacher_class_ownership(user, params)
+    class_ownership = verify_class_ownership(user, params)
     return error(class_ownership[:error]) if class_ownership[:error]
-    teacher_clazz = class_ownership[:teacher_clazz]
+    class_to_copy = class_ownership[:clazz]
 
-    class_to_copy = teacher_clazz.clazz
     new_clazz = Portal::Clazz.new(
       :name => params[:name],
       :class_word => params[:classWord],
@@ -95,6 +96,19 @@ def render_teacher_clazz(teacher_clazz)
     }, :status => :ok
   end
 
+  def verify_class_ownership(user, params)
+    clazz = Portal::Clazz.find(params[:id])
+    if !clazz
+      return {error: 'The requested class was not found'}
+    end
+
+    if !clazz.is_teacher?(user)
+      return {error: 'You are not a teacher of the requested class'}
+    end
+
+    return {clazz: clazz}
+  end
+
   def verify_teacher_class_ownership(user, params)
     teacher_clazz = Portal::TeacherClazz.find_by_id(params[:id])
     if !teacher_clazz

rails/spec/controllers/api/v1/teacher_clazzes_controller_spec.rb

@@ -13,13 +13,9 @@
 
   let(:clazz2)             { FactoryBot.create(:portal_clazz, name: 'test class #2', teachers: [teacher]) }
   let(:clazz3)             { FactoryBot.create(:portal_clazz, name: 'test class #3', teachers: [teacher]) }
-  let(:teacher_clazz)      { FactoryBot.create(:portal_teacher_clazz, teacher: teacher, clazz: clazz) }
-  let(:teacher_clazz2)     { FactoryBot.create(:portal_teacher_clazz, teacher: teacher, clazz: clazz2) }
-  let(:teacher_clazz3)     { FactoryBot.create(:portal_teacher_clazz, teacher: teacher, clazz: clazz3) }
 
   let(:other_teacher)       { FactoryBot.create(:portal_teacher) }
   let(:other_clazz)         { FactoryBot.create(:portal_clazz, name: 'other class', teachers: [other_teacher]) }
-  let(:other_teacher_clazz) { FactoryBot.create(:portal_teacher_clazz, teacher: other_teacher, clazz: other_clazz) }
 
   describe "GET #show" do
     before (:each) do
@@ -34,7 +30,8 @@
       expect(JSON.parse(response.body)["message"]).to eq "The requested teacher class was not found"
     end
 
-    it "returns a 200 code for a valid class" do
+    it "returns a 200 code for a valid teacher class" do
+      teacher_clazz = Portal::TeacherClazz.where(clazz_id: clazz.id, teacher_id: teacher.id).first
       get :show, params: { id: teacher_clazz.id }
       expect(response).to have_http_status(:ok)
       expect(JSON.parse(response.body)["data"]).to eq({
@@ -80,78 +77,73 @@
     it "should fail when ids are invalid" do
       post :sort, params: { ids: [0] }
       expect(response).to have_http_status(:bad_request)
-      expect(JSON.parse(response.body)["message"]).to eq "Invalid teacher class id: 0"
+      expect(JSON.parse(response.body)["message"]).to eq "Invalid class id: 0"
     end
 
     it "should fail when ids are to classes that the teacher doesn't own" do
-      post :sort, params: { ids: [other_teacher_clazz.id] }
+      post :sort, params: { ids: [other_clazz.id] }
       expect(response).to have_http_status(:bad_request)
-      expect(JSON.parse(response.body)["message"]).to eq "You are not a teacher of class: #{other_teacher_clazz.id}"
+      expect(JSON.parse(response.body)["message"]).to eq "You are not a teacher of class: #{other_clazz.id}"
     end
 
     it "should succeed" do
-      post :sort, params: { ids: [teacher_clazz3.id, teacher_clazz.id, teacher_clazz2.id] }
-      teacher_clazz.reload
-      teacher_clazz2.reload
-      teacher_clazz3.reload
+      post :sort, params: { ids: [clazz3.id, clazz.id, clazz2.id] }
       expect(response).to have_http_status(:ok)
-      expect(teacher_clazz3.position).to eq 1
-      expect(teacher_clazz.position).to eq 2
-      expect(teacher_clazz2.position).to eq 3
-
-      post :sort, params: { ids: [teacher_clazz2.id, teacher_clazz3.id, teacher_clazz.id] }
-      teacher_clazz.reload
-      teacher_clazz2.reload
-      teacher_clazz3.reload
+      expect(Portal::TeacherClazz.where(clazz_id: clazz3.id, teacher_id: teacher.id).first.position).to eq 1
+      expect(Portal::TeacherClazz.where(clazz_id: clazz.id, teacher_id: teacher.id).first.position).to eq 2
+      expect(Portal::TeacherClazz.where(clazz_id: clazz2.id, teacher_id: teacher.id).first.position).to eq 3
+
+      post :sort, params: { ids: [clazz2.id, clazz3.id, clazz.id] }
       expect(response).to have_http_status(:ok)
-      expect(teacher_clazz2.position).to eq 1
-      expect(teacher_clazz3.position).to eq 2
-      expect(teacher_clazz.position).to eq 3
+      expect(Portal::TeacherClazz.where(clazz_id: clazz2.id, teacher_id: teacher.id).first.position).to eq 1
+      expect(Portal::TeacherClazz.where(clazz_id: clazz3.id, teacher_id: teacher.id).first.position).to eq 2
+      expect(Portal::TeacherClazz.where(clazz_id: clazz.id, teacher_id: teacher.id).first.position).to eq 3
     end
   end
 
   describe "#copy" do
     before :each do
+      clazz
       sign_in teacher.user
     end
 
     it "should fail when id is a class that the teacher doesn't own" do
-      post :copy, params: { id: other_teacher_clazz.id }
+      post :copy, params: { id: other_clazz.id }
       expect(response).to have_http_status(:bad_request)
       expect(JSON.parse(response.body)["message"]).to eq "You are not a teacher of the requested class"
     end
 
     it "should fail when name is blank" do
-      post :copy, params: { id: teacher_clazz.id, name: "", classWord: "copyofclazz", description: "test of copy" }
+      post :copy, params: { id: clazz.id, name: "", classWord: "copyofclazz", description: "test of copy" }
       expect(response).to have_http_status(:bad_request)
       expect(JSON.parse(response.body)["message"]).to eq "Name can't be blank"
     end
 
     it "should fail when class word is blank" do
-      post :copy, params: { id: teacher_clazz.id, name: "Copy of clazz for unit test", classWord: "", description: "test of copy" }
+      post :copy, params: { id: clazz.id, name: "Copy of clazz for unit test", classWord: "", description: "test of copy" }
       expect(response).to have_http_status(:bad_request)
       expect(JSON.parse(response.body)["message"]).to eq "Class word can't be blank"
     end
 
     it "should fail when class word is taken" do
-      post :copy, params: { id: teacher_clazz.id, name: "Copy of clazz for unit test", classWord: clazz.class_word, description: "test of copy" }
+      post :copy, params: { id: clazz.id, name: "Copy of clazz for unit test", classWord: clazz.class_word, description: "test of copy" }
       expect(response).to have_http_status(:bad_request)
       expect(JSON.parse(response.body)["message"]).to eq "Class word has already been taken"
     end
 
     it "should succeed when fields are valid" do
-      post :copy, params: { id: teacher_clazz.id, name: "Copy of clazz for unit test", classWord: "copyofclazz", description: "test of copy" }
+      post :copy, params: { id: clazz.id, name: "Copy of clazz for unit test", classWord: "copyofclazz", description: "test of copy" }
       expect(response).to have_http_status(:ok)
 
       data = JSON.parse(response.body)["data"]
       expect(data["id"]).not_to be_nil
       expect(data["name"]).to eq "Copy of clazz for unit test"
       expect(data["class_word"]).to eq "copyofclazz"
       expect(data["description"]).to eq "test of copy"
-      expect(data["position"]).to eq 4
+      expect(data["position"]).to eq Portal::TeacherClazz.where(teacher_id: teacher.id).count
 
       copy_teacher_clazz = Portal::TeacherClazz.find_by_id(data["id"])
-      expect(copy_teacher_clazz.id).not_to be teacher_clazz.id
+      expect(copy_teacher_clazz.id).not_to be Portal::TeacherClazz.where(clazz_id: clazz.id, teacher_id: teacher.id).first
 
       copy_clazz = copy_teacher_clazz.clazz
       expect(copy_clazz.id).not_to be clazz.id