Merge pull request #1878 from openstax/role_id_urls

Replace become API with (/role/:role_id) or ?role_id= in URLs that use it

Author: nathanstitt

app/access_policies/note_access_policy.rb

@@ -3,7 +3,7 @@ def self.action_allowed?(action, requestor, note)
     return false unless note.present? && requestor.is_human?
 
     case action.to_sym
-    when :index, :read
+    when :index
       true
     when :create, :update, :destroy
       note.role.user_profile_id == requestor.id

app/access_policies/teacher_student_access_policy.rb

@@ -4,9 +4,7 @@ def self.action_allowed?(action, requestor, teacher_student)
 
     case action
     when :show
-      teacher_student.role.user_profile_id == requestor.id &&
-        UserIsCourseTeacher[user: requestor, course: teacher_student.course] &&
-        !teacher_student.deleted? &&
+      UserIsCourseTeacher[user: requestor, course: teacher_student.course] &&
         !teacher_student.period.nil? &&
         !teacher_student.period.archived?
     else

app/controllers/admin/salesforce_controller.rb

@@ -1,17 +1,17 @@
 module Admin
   class SalesforceController < BaseController
-    def actions
+    def show
     end
 
-    def update_salesforce
+    def update
       outputs = PushSalesforceCourseStats.call(allow_error_email: true)
 
       flash[:notice] = "Ran on #{outputs[:num_courses]} course(s); " +
                        "updated #{outputs[:num_updates]} course(s); " +
                        "#{outputs[:num_errors]} error(s) occurred; " +
                        "#{outputs[:num_skips]} skip(s)"
 
-      redirect_to actions_admin_salesforce_path
+      redirect_to admin_salesforce_path
     end
   end
 end

app/controllers/admin/students_controller.rb

@@ -21,16 +21,16 @@ def update
     end
   end
 
-  def refund
+  def destroy
     student = CourseMembership::Models::Student.find(params[:id])
-    RefundPayment[uuid: student.uuid]
-    redirect_to edit_admin_course_path(student.course, anchor: "roster")
+    CourseMembership::InactivateStudent[student: student]
+    redirect_to edit_admin_course_path(student.course) + '#roster'
   end
 
-  def drop
+  def refund
     student = CourseMembership::Models::Student.find(params[:id])
-    CourseMembership::InactivateStudent[student: student]
-    redirect_to edit_admin_course_path(student.course) + '#roster'
+    RefundPayment[uuid: student.uuid]
+    redirect_to edit_admin_course_path(student.course, anchor: "roster")
   end
 
   def restore

app/controllers/admin/targeted_contracts_controller.rb

@@ -1,6 +1,6 @@
 class Admin::TargetedContractsController < Admin::BaseController
 
-  before_action :load_district_targets, only: [:new]
+  before_action :load_district_targets, only: :new
 
   def index
     @contracts = Legal::GetTargetedContracts[]

app/controllers/admin/teachers_controller.rb

@@ -15,14 +15,14 @@ def teachers
     redirect_to edit_admin_course_path(course, anchor: 'teachers')
   end
 
-  def delete
+  def destroy
     teacher = CourseMembership::Models::Teacher.find(params[:id])
     teacher.destroy
     flash[:notice] = "Teacher \"#{teacher.role.name}\" removed from course."
     redirect_to edit_admin_course_path(teacher.course, anchor: 'teachers')
   end
 
-  def undelete
+  def restore
     teacher = CourseMembership::Models::Teacher.find(params[:id])
     teacher.restore
     flash[:notice] = "Teacher \"#{teacher.role.name}\" readded to course."

app/controllers/admin/users_controller.rb

@@ -1,7 +1,7 @@
 class Admin::UsersController < Admin::BaseController
   include Manager::SearchUsers
 
-  before_action :get_user, only: [:edit, :update, :become]
+  before_action :get_user, only: [ :edit, :update, :become ]
 
   def create
     handle_with(

app/controllers/api/v1/courses_controller.rb

@@ -3,8 +3,8 @@ class Api::V1::CoursesController < Api::V1::ApiController
     :name, :is_preview, :num_sections, :catalog_offering_id
   ]
 
-  before_action :get_course, only: [:show, :update, :dashboard, :cc_dashboard, :roster, :clone]
-  before_action :error_if_student_and_needs_to_pay, only: [:dashboard]
+  before_action :get_course, only: [ :show, :update, :dashboard, :cc_dashboard, :roster, :clone ]
+  before_action :error_if_student_and_needs_to_pay, only: :dashboard
 
   resource_description do
     api_versions "v1"
@@ -16,20 +16,16 @@ class Api::V1::CoursesController < Api::V1::ApiController
 
   api :GET, '/courses', 'Returns courses'
   description <<-EOS
-    Returns courses in the system, and the user who requested them is shown
-    their own roles related to their courses
+    Returns courses in the system,
+    and the user who requested them is shown their own roles in each course
     #{json_schema(Api::V1::CoursesRepresenter, include: :readable)}
   EOS
   def index
     OSU::AccessPolicy.require_action_allowed!(
       :index, current_api_user, CourseProfile::Models::Course
     )
 
-    course_infos = CollectCourseInfo[
-      user: current_human_user, current_roles_hash: current_roles_hash
-    ]
-
-    respond_with course_infos, represent_with: Api::V1::CoursesRepresenter
+    respond_with collect_course_info, represent_with: Api::V1::CoursesRepresenter
   end
 
   api :POST, '/courses', 'Creates a new course'
@@ -236,15 +232,20 @@ def get_course
     @course = CourseProfile::Models::Course.find(params[:id])
   end
 
-  def collect_course_info(course:)
-    CollectCourseInfo[
-      courses: course, user: current_human_user, current_roles_hash: current_roles_hash
-    ].first
+  def collect_course_info(course: nil)
+    args = { user: current_human_user }
+
+    if course.nil?
+      CollectCourseInfo[args]
+    else
+      args[:courses] = course
+      CollectCourseInfo[args].first
+    end
   end
 
   def get_course_role(course:)
     result = ChooseCourseRole.call(
-      user: current_human_user, course: course, current_roles_hash: current_roles_hash
+      user: current_human_user, course: course, role_id: params[:role_id]
     )
     errors = result.errors
     raise(SecurityTransgression, :invalid_role) unless errors.empty?

app/controllers/api/v1/guides_controller.rb

@@ -18,20 +18,21 @@ class GuidesController < ApiController
         #{json_schema(Api::V1::CourseGuidePeriodRepresenter, include: :readable)}
       EOS
       def student
+        # We don't use the get_course_role method when a role_id is given
+        # because we allow teachers to look at any student's performance forecast
+        # not just their own roles'
+        # We trust the AccessPolicy for access control in this case
         role = if params[:role_id].blank?
-          get_course_role(course: @course, allowed_role_types: [:student, :teacher_student])
+          get_course_role(course: @course, allowed_role_types: [ :student, :teacher_student ])
         else
           Entity::Role.where(
             role_type: Entity::Role.role_types.values_at(:student, :teacher_student)
           ).find(params[:role_id])
         end
 
-        student = role.course_member
-
-        OSU::AccessPolicy.require_action_allowed!(:show, current_api_user, student)
+        OSU::AccessPolicy.require_action_allowed!(:show, current_api_user, role.course_member)
 
         guide = GetStudentGuide[role: role]
-
         respond_with guide, represent_with: Api::V1::CourseGuidePeriodRepresenter
       end
 
@@ -42,7 +43,9 @@ def student
       EOS
       def teacher
         role = get_course_role(course: @course, allowed_role_types: :teacher)
+
         OSU::AccessPolicy.require_action_allowed!(:show, current_api_user, role.teacher)
+
         guide = GetTeacherGuide[role: role]
         respond_with guide, represent_with: Api::V1::TeacherCourseGuideRepresenter
       end
@@ -56,10 +59,9 @@ def get_course
       def get_course_role(course:, allowed_role_types:)
         result = ChooseCourseRole.call(
           user: current_human_user, course: course,
-          current_roles_hash: current_roles_hash, allowed_role_types: allowed_role_types
+          role_id: params[:role_id], allowed_role_types: allowed_role_types
         )
-        errors = result.errors
-        raise(SecurityTransgression, :invalid_role) unless errors.empty?
+        raise(SecurityTransgression, :invalid_role) unless result.errors.empty?
         result.outputs.role
       end
     end

app/controllers/api/v1/notes_controller.rb

@@ -1,56 +1,53 @@
 # coding: utf-8
 class Api::V1::NotesController < Api::V1::ApiController
 
-  before_action :get_course_role
-  before_action :get_note, only: [:update, :destroy]
+  before_action :get_course, except: [ :update, :destroy ]
+  before_action :get_user_roles_and_page_uuid, only: [ :index, :highlighted_sections ]
+  before_action :choose_course_role, only: :create
+  before_action :get_note, only: [ :update, :destroy ]
 
   resource_description do
     api_versions "v1"
     short_description 'Represents a note added by the student on a course'
     description <<-EOS
-      Stores text selection (notes) on a course’s content.  Notes are generated
-      by users as they highlight content, and then are fetched and re-stored when
-      the content is reloaded.
+      Stores text selection (notes) on a course’s content.
+      Notes are generated by users as they highlight content,
+      and then are fetched and re-stored when the content is reloaded.
     EOS
   end
 
   ####################################################################
   ## index                                                          ##
   ####################################################################
-  api :GET, '/api/courses/:course_id/notes/:chapter.:section', 'Lists all user notes for the given course/page/section'
+  api :GET, '/api/courses/:course_id/notes/:chapter.:section', 'Lists all notes'
   description <<-EOS
-    list all the notes added by the student to the given course, page and section
+    Lists all the notes added by the student to the given course, page and section
+
     #{json_schema(Api::V1::NotesRepresenter, include: :readable)}
   EOS
   def index
-    page_ids = Content::Models::Page
-               .joins(:notes)
-               .where(notes: { role: @role })
-               .book_location(params[:chapter], params[:section])
-               .pluck(:id)
-    notes = Content::Models::Note.where(role: @role, content_page_id: page_ids)
+    notes = Content::Models::Note.joins(:page).where(role: @roles, page: { uuid: @page_uuid })
     respond_with notes, represent_with: Api::V1::NotesRepresenter
   end
 
-  ###############################################################
-  # post
-  ###############################################################
-  api :POST, '/api/courses/:course_id/notes/:chapter.:section', 'Creates a Note'
+  ####################################################################
+  ## create                                                         ##
+  ####################################################################
+  api :POST, '/api/courses/:course_id/notes/:chapter.:section(/role/:role_id)', 'Creates a note'
   description <<-EOS
-    Create a new note for the given course, chapter and section
+    Creates a new note for the given course, chapter and section
+
     #{json_schema(Api::V1::NoteRepresenter, include: :readable)}
   EOS
   def create
     note = Content::Models::Note.new(role: @role, content_page_id: params[:page_id])
     consume!(note, represent_with: Api::V1::NoteRepresenter)
     OSU::AccessPolicy.require_action_allowed!(:create, current_api_user, note)
-    note.page = @course.ecosystem.pages.book_location(
-      params[:chapter], params[:section],
-    ).first!
+    note.page = @course.ecosystem.pages.book_location(params[:chapter], params[:section]).first!
     if note.save
       respond_with note, responder: ResponderWithPutPatchDeleteContent,
-                   represent_with: Api::V1::NoteRepresenter,
-                   location: nil
+                         represent_with: Api::V1::NoteRepresenter,
+                         location: nil
     else
       render_api_errors(note.errors)
     end
@@ -59,9 +56,10 @@ def create
   ####################################################################
   ## update                                                         ##
   ####################################################################
-  api :PUT, '/api/courses/:course_id/notes/:chapter.:section/:id', 'Updates a Note'
+  api :PUT, '/api/courses/:course_id/notes/:chapter.:section/:id', 'Updates a note'
   description <<-EOS
-    Updates a note for the given course, chapter, section and id
+    Updates the note with the given id
+
     #{json_schema(Api::V1::NoteRepresenter, include: :readable)}
   EOS
   def update
@@ -77,50 +75,60 @@ def update
   end
 
   ####################################################################
-  ## delete                                                         ##
+  ## destroy                                                        ##
   ####################################################################
-  api :DELETE, '/api/courses/:course_id/notes/:chapter.:section/:id', 'Deletes the note from the students course'
+  api :DELETE, '/api/courses/:course_id/notes/:chapter.:section/:id', 'Deletes a note'
   description <<-EOS
-    Deletes the note from the student's course with the provided :id
+    Deletes the note with the given id
   EOS
   def destroy
     OSU::AccessPolicy.require_action_allowed!(:destroy, current_api_user, @note)
-    @note.destroy!
+    @note.destroy
     render_api_errors(@note.errors) || head(:ok)
   end
 
-  api :GET, '/api/courses/:course_id/highlighted_sections', 'Lists a notes summary from the students course'
+  ####################################################################
+  ## highlighted_sections                                           ##
+  ####################################################################
+  api :GET, '/api/courses/:course_id/highlighted_sections',
+            'Shows a summary of sections highlighted by the student'
   description <<-EOS
-    list all the notes added by the student to a course
+    List all sections highlighted by the student
+
     #{json_schema(Api::V1::HighlightRepresenter, include: :readable)}
   EOS
   def highlighted_sections
-    pages = Content::Models::Page
-              .select('content_pages.id, content_pages.title, content_pages.book_location, count(*) as notes_count')
-              .group(:id)
-              .joins(:notes)
-              .where(notes: { role: @role })
+    pages = Content::Models::Page.select(:id, :title, :book_location, 'count(*) as notes_count')
+                                 .joins(:notes)
+                                 .where(uuid: @page_uuid, notes: { role: @roles })
+                                 .group(:id)
     respond_with pages, represent_with: Api::V1::HighlightRepresenter
   end
 
   protected
 
-  def get_note
-    @note = Content::Models::Note.find_by(
-      id: params[:id], role: @role
-    )
+  def get_course
+    @course = CourseProfile::Models::Course.find(params[:course_id])
   end
 
-  def get_course_role
-    @course = CourseProfile::Models::Course.find(params[:course_id])
+  def get_user_roles_and_page_uuid
+    @roles = current_human_user.roles
+    @page_uuid = @course.ecosystem.pages.book_location(params[:chapter], params[:section])
+                                        .pluck(:uuid)
+                                        .first || raise(ActiveRecord::RecordNotFound)
+  end
 
+  def choose_course_role
     result = ChooseCourseRole.call(
-      user: current_human_user, course: @course, current_roles_hash: current_roles_hash
+      user: current_human_user, course: @course, role_id: params[:role_id]
     )
-    errors = result.errors
-    raise(SecurityTransgression, :invalid_role) unless errors.empty?
+    raise(SecurityTransgression, :invalid_role) unless result.errors.empty?
 
     @role = result.outputs.role
   end
 
+  def get_note
+    @note = Content::Models::Note.find(params[:id])
+  end
+
 end