code-dot-org · cforkish · Sep 9, 2020 · Sep 9, 2020
diff --git a/Gemfile b/Gemfile
@@ -235,7 +235,7 @@ gem 'paranoia'
 gem 'petit', github: 'code-dot-org/petit'  # For URL shortening

 # JSON model serializer for REST APIs.
-gem 'active_model_serializers', '~> 0.10.0'
+gem 'active_model_serializers', github: 'rails-api/active_model_serializers', ref: '2962f3f64e7c672bfb5a13a8f739b5db073e5473'

 # AWS SDK and associated service APIs.
 gem 'aws-sdk-acm'

diff --git a/Gemfile.lock b/Gemfile.lock
@@ -54,6 +54,14 @@ GIT
  specs:
    full-name-splitter (0.1.2)

+GIT
+  remote: https://github.com/rails-api/active_model_serializers.git
+  revision: 2962f3f64e7c672bfb5a13a8f739b5db073e5473
+  ref: 2962f3f64e7c672bfb5a13a8f739b5db073e5473
+  specs:
+    active_model_serializers (0.10.0.pre)
+      activemodel (>= 4.0)
+
 GIT
  remote: https://github.com/wjordan/gctools.git
  revision: 729c6bbb32c4bcc2ebfed16543991e4d126f09cc
@@ -227,11 +235,6 @@ GEM
      erubis (~> 2.7.0)
      rails-dom-testing (~> 2.0)
      rails-html-sanitizer (~> 1.0, >= 1.0.3)
-    active_model_serializers (0.10.10)
-      actionpack (>= 4.1, < 6.1)
-      activemodel (>= 4.1, < 6.1)
-      case_transform (>= 0.2)
-      jsonapi-renderer (>= 0.1.1.beta1, < 0.3)
    active_record_query_trace (1.5.3)
    active_record_union (1.3.0)
      activerecord (>= 4.0)
@@ -340,8 +343,6 @@ GEM
    brakeman (4.5.0)
    builder (3.2.3)
    cancancan (1.15.0)
-    case_transform (0.2)
-      activesupport
    childprocess (0.9.0)
      ffi (~> 1.0, >= 1.0.11)
    chronic (0.10.2)
@@ -526,7 +527,6 @@ GEM
      bindata
    json-schema (2.8.1)
      addressable (>= 2.4)
-    jsonapi-renderer (0.2.2)
    jsonapi-serializers (1.0.0)
      activesupport
    jumphash (0.1.0)
@@ -905,7 +905,7 @@ PLATFORMS
 DEPENDENCIES
  StreetAddress
  acmesmith (~> 2.3.1)
-  active_model_serializers (~> 0.10.0)
+  active_model_serializers!
  active_record_query_trace
  active_record_union
  activerecord-import

diff --git a/dashboard/app/controllers/api/v1/pd/applications_controller.rb b/dashboard/app/controllers/api/v1/pd/applications_controller.rb
@@ -63,8 +63,12 @@ def quick_view

      respond_to do |format|
        format.json do
-          prefetched_applications = prefetch(applications, role: role)
-          render json: prefetched_applications, each_serializer: ApplicationQuickViewSerializer
+          serialized_applications = prefetch_and_serialize(
+            applications,
+            role: role,
+            serializer: ApplicationQuickViewSerializer
+          )
+          render json: serialized_applications
        end
        format.csv do
          csv_text = get_csv_text applications, role
@@ -93,8 +97,13 @@ def cohort_view

      respond_to do |format|
        format.json do
-          prefetched_applications = prefetch(applications, role: role)
-          render json: prefetched_applications, each_serializer: serializer, scope: {user: current_user}
+          serialized_applications = prefetch_and_serialize(
+            applications,
+            role: role,
+            serializer: serializer,
+            scope: {user: current_user}
+          )
+          render json: serialized_applications
        end
        format.csv do
          csv_text = get_csv_text applications, role
@@ -200,7 +209,8 @@ def search
        user: user
      )

-      render json: filtered_applications, each_serializer: ApplicationSearchSerializer
+      serialized_applications = filtered_applications.map {|a| ApplicationSearchSerializer.new(a).attributes}
+      render json: serialized_applications
    end

    private
@@ -294,10 +304,16 @@ def get_optional_columns(_regional_partner_value)
      {registered_workshop: false}
    end

+    def prefetch_and_serialize(applications, role: nil, serializer:, scope: {})
+      prefetch applications, role: role
+      applications.map do |application|
+        serializer.new(application, scope: scope).attributes
+      end
+    end
+
    def prefetch(applications, role: nil)
      type = TYPES_BY_ROLE[role.try(&:to_sym)]
      type.prefetch_associated_models applications
-      applications
    end
  end
 end
diff --git a/dashboard/app/controllers/api/v1/pd/regional_partner_workshops_controller.rb b/dashboard/app/controllers/api/v1/pd/regional_partner_workshops_controller.rb
@@ -18,24 +18,26 @@ def find

    # Find the matching partner, even if it has no workshops
    partner = @partners.find_by_region(zip_code, state) || RegionalPartner.find_by_region(zip_code, state)
-    # To preserve existing behavior after upgrading to ActiveModelSerializers 10.x,
-    # initialize partner to an object with nil values if not found.
-    partner ||= RegionalPartner.new
-
-    render json: partner,
-           serializer: Api::V1::Pd::RegionalPartnerWorkshopsSerializer,
-           scope: {course: @course, subject: @subject}
+    render json: serialize_partner_workshops(partner)
  end

  # GET /api/v1/pd/regional_partner_workshops
  def index
-    render json: @partners,
-           each_serializer: Api::V1::Pd::RegionalPartnerWorkshopsSerializer,
-           scope: {course: @course, subject: @subject}
+    render json: @partners.map {|p| serialize_partner_workshops(p)}
  end

  private

+  def serialize_partner_workshops(partner)
+    # The scope is not being passed to the serializer with `render json: serializer:` syntax,
+    # so initialize this explicitly.
+    # TODO (Andrew): Look into updating our very outdated version of ActiveModelSerializers
+    Api::V1::Pd::RegionalPartnerWorkshopsSerializer.new(
+      partner,
+      scope: {course: @course, subject: @subject}
+    ).attributes
+  end
+
  def get_filtered_workshops
    @partners = RegionalPartner.includes(:pd_workshops)


diff --git a/dashboard/app/controllers/api/v1/pd/workshop_enrollments_controller.rb b/dashboard/app/controllers/api/v1/pd/workshop_enrollments_controller.rb
@@ -21,12 +21,13 @@ def authorize_update_scholarship_info!

  # GET /api/v1/pd/workshops/1/enrollments
  def index
+    response = render_to_json @workshop.enrollments, each_serializer: Api::V1::Pd::WorkshopEnrollmentSerializer
+
    respond_to do |format|
      format.json do
-        render json: @workshop.enrollments, each_serializer: Api::V1::Pd::WorkshopEnrollmentSerializer
+        render json: response
      end
      format.csv do
-        response = render_to_json @workshop.enrollments, each_serializer: Api::V1::Pd::WorkshopEnrollmentSerializer
        send_as_csv_attachment response, 'workshop_enrollments.csv'
      end
    end
@@ -80,7 +81,8 @@ def create
  # POST /api/v1/pd/enrollment/:enrollment_id/scholarship_info
  def update_scholarship_info
    @enrollment.update_scholarship_status(params[:scholarship_status])
-    render json: @enrollment, serializer: Api::V1::Pd::WorkshopEnrollmentSerializer
+    serialized_enrollment = Api::V1::Pd::WorkshopEnrollmentSerializer.new(@enrollment).attributes
+    render json: serialized_enrollment
  end

  # DELETE /api/v1/pd/workshops/1/enrollments/1

diff --git a/dashboard/app/controllers/api/v1/teacher_feedbacks_controller.rb b/dashboard/app/controllers/api/v1/teacher_feedbacks_controller.rb
@@ -49,7 +49,7 @@ def count
      User.find(feedback.teacher_id).authorized_teacher?
    end

-    render json: @all_unseen_feedbacks.count
+    render json: @all_unseen_feedbacks.count, each_serializer: Api::V1::TeacherFeedbackSerializer
  end

  # POST /teacher_feedbacks

diff --git a/dashboard/app/controllers/api/v1/users_controller.rb b/dashboard/app/controllers/api/v1/users_controller.rb
@@ -60,7 +60,7 @@ def get_donor_teacher_banner_details

  # GET /api/v1/users/<user_id>/school_donor_name
  def get_school_donor_name
-    render json: @user.school_donor_name.nil? ? 'null' : @user.school_donor_name.inspect
+    render json: @user.school_donor_name
  end

  # POST /api/v1/users/<user_id>/using_text_mode

diff --git a/dashboard/app/controllers/script_levels_controller.rb b/dashboard/app/controllers/script_levels_controller.rb
@@ -160,7 +160,7 @@ def hidden_stage_ids

    stage_ids = current_user ? current_user.get_hidden_stage_ids(params[:script_id]) : []

-    render json: stage_ids.map(&:to_s)
+    render json: stage_ids
  end

  # toggles whether or not a stage is hidden for a section

diff --git a/dashboard/app/serializers/api/v1/pd/cohort_view_serializer_base.rb b/dashboard/app/serializers/api/v1/pd/cohort_view_serializer_base.rb
@@ -1,23 +1,26 @@
 class Api::V1::Pd::CohortViewSerializerBase < ActiveModel::Serializer
-  # Declare attributes individually so we can make :locked a conditional attribute
-  attribute :id
-  attribute :date_accepted
-  attribute :applicant_name
-  attribute :district_name
-  attribute :school_name
-  attribute :email
-  attribute :assigned_workshop
-  attribute :registered_workshop
-  attribute :status
-  attribute :notes
-  attribute :notes_2
-  attribute :notes_3
-  attribute :notes_4
-  attribute :notes_5
-  attribute :locked, if: :include_locked?
+  attributes(
+    :id,
+    :date_accepted,
+    :applicant_name,
+    :district_name,
+    :school_name,
+    :email,
+    :assigned_workshop,
+    :registered_workshop,
+    :status,
+    :notes,
+    :notes_2,
+    :notes_3,
+    :notes_4,
+    :notes_5
+  )

-  def locked
-    object.locked?
+  # Dynamically add locked where applicable
+  def attributes(attrs = {})
+    super(attrs).tap do |data|
+      data[:locked] = object.locked? if object.class.can_see_locked_status?(@scope[:user])
+    end
  end

  def email
@@ -33,8 +36,4 @@ def registered_workshop
      object.registered_workshop? ? 'Yes' : 'No'
    end
  end
-
-  def include_locked?
-    object.class.can_see_locked_status?(scope[:user])
-  end
 end
diff --git a/dashboard/app/serializers/api/v1/pd/enrollment_flat_attendance_serializer.rb b/dashboard/app/serializers/api/v1/pd/enrollment_flat_attendance_serializer.rb
@@ -1,6 +1,6 @@
 # Write a flat list of workshop attendance by session for the enrollment
 class Api::V1::Pd::EnrollmentFlatAttendanceSerializer < ActiveModel::Serializer
-  attributes :first_name, :last_name, :email, :district_name, :school, :role, :grades_teaching, :cdo_scholarship, :other_scholarship
+  attributes :first_name, :last_name, :email, :district_name, :school, :role, :grades_teaching

  def district_name
    object.school_info&.school_district&.name
@@ -10,21 +10,15 @@ def school
    object.school_info&.school&.name || object.school_info&.school_name || object.school
  end

-  def cdo_scholarship
-    object.scholarship_status == Pd::ScholarshipInfoConstants::YES_CDO ? 'Yes' : ''
-  end
-
-  def other_scholarship
-    object.scholarship_status == Pd::ScholarshipInfoConstants::YES_OTHER ? 'Yes' : ''
-  end
-
  # Add dynamic attributes for each session's date and attendance
-  def attributes(requested_attrs = nil, reload = false)
-    super(requested_attrs, reload).tap do |data|
+  def attributes(attrs = {})
+    super(attrs).tap do |data|
      object.workshop.sessions.each_with_index do |session, i|
        data["session_#{i + 1}_date".to_sym] = session.formatted_date
        data["session_#{i + 1}_attendance".to_sym] = session.attendances.where(pd_enrollment_id: object.id).exists?
      end
+      data[:cdo_scholarship] = object.scholarship_status == Pd::ScholarshipInfoConstants::YES_CDO ? 'Yes' : ''
+      data[:other_scholarship] = object.scholarship_status == Pd::ScholarshipInfoConstants::YES_OTHER ? 'Yes' : ''
    end
  end
 end
diff --git a/dashboard/app/serializers/api/v1/pd/facilitator_application_cohort_view_serializer.rb b/dashboard/app/serializers/api/v1/pd/facilitator_application_cohort_view_serializer.rb
@@ -1,9 +1,10 @@
 module Api::V1::Pd
  class FacilitatorApplicationCohortViewSerializer < CohortViewSerializerBase
-    # Declare attributes individually instead of using attributes list, to preserve attributes declared on base class
-
-    attribute :assigned_fit
-    attribute :registered_fit
+    attributes(
+      *superclass._attributes,
+      :assigned_fit,
+      :registered_fit
+    )

    def assigned_fit
      object.fit_workshop.try(&:date_and_location_name)

diff --git a/dashboard/app/serializers/api/v1/pd/regional_partner_workshops_serializer.rb b/dashboard/app/serializers/api/v1/pd/regional_partner_workshops_serializer.rb
@@ -2,17 +2,17 @@ class Api::V1::Pd::RegionalPartnerWorkshopsSerializer < ActiveModel::Serializer
  attributes :id, :name, :group, :workshops, :has_csf

  def workshops
-    return nil if object.id.nil?
-
-    workshops = object.pd_workshops.future
-    workshops = workshops.where(course: @scope[:course]) if @scope.try(:[], :course)
-    workshops = workshops.where(subject: @scope[:subject]) if @scope.try(:[], :subject)
-    workshops.map do |workshop|
-      {
-        id: workshop.id,
-        dates: workshop.friendly_date_range,
-        location: workshop.location_address
-      }
+    object.try do |partner|
+      workshops = partner.pd_workshops.future
+      workshops = workshops.where(course: @scope[:course]) if @scope.try(:[], :course)
+      workshops = workshops.where(subject: @scope[:subject]) if @scope.try(:[], :subject)
+      workshops.map do |workshop|
+        {
+          id: workshop.id,
+          dates: workshop.friendly_date_range,
+          location: workshop.location_address
+        }
+      end
    end
  end
 end
diff --git a/dashboard/app/serializers/api/v1/pd/teacher_application_cohort_view_serializer.rb b/dashboard/app/serializers/api/v1/pd/teacher_application_cohort_view_serializer.rb
@@ -1,8 +1,6 @@
 module Api::V1::Pd
  class TeacherApplicationCohortViewSerializer < CohortViewSerializerBase
-    # Declare attributes individually instead of using attributes list, to preserve attributes declared on base class
-    attribute :friendly_scholarship_status
-    attribute :registered_workshop_id
+    attributes(*superclass._attributes, :friendly_scholarship_status, :registered_workshop_id)

    def registered_workshop_id
      if object.workshop.try(:local_summer?)

diff --git a/dashboard/app/serializers/api/v1/teacher_feedback_serializer.rb b/dashboard/app/serializers/api/v1/teacher_feedback_serializer.rb
@@ -20,6 +20,8 @@
 class Api::V1::TeacherFeedbackSerializer < ActiveModel::Serializer
  attributes :id, :teacher_name, :student_id, :level_id, :script_level_id, :comment, :performance, :created_at

+  private
+
  def teacher_name
    object.teacher.name
  end

diff --git a/dashboard/config/initializers/active_model_serializers.rb b/dashboard/config/initializers/active_model_serializers.rb
@@ -1 +1,15 @@
-ActiveModel::Serializer.config.adapter = :attributes
+# Patch to allow serializing String and Hash objects by default
+class StringSerializer < ActiveModel::Serializer
+  def attributes(obj)
+    @object.to_s
+  end
+end
+class IntegerSerializer < StringSerializer; end
+class HashSerializer < ActiveModel::Serializer
+  def attributes(obj)
+    @object.as_json
+  end
+end
+class ActiveModel::ErrorsSerializer < HashSerializer; end
+
+ActiveModel::Serializer.config.adapter = :Json
diff --git a/dashboard/test/controllers/api/v1/pd/workshop_enrollments_controller_test.rb b/dashboard/test/controllers/api/v1/pd/workshop_enrollments_controller_test.rb
@@ -109,18 +109,6 @@ class Api::V1::Pd::WorkshopEnrollmentsControllerTest < ::ActionController::TestC
    assert_equal @enrollment.email, response_json[0]['email']
  end

-  test 'facilitators can see enrollments in their workshops in csv format' do
-    sign_in @facilitator
-    get :index, params: {workshop_id: @workshop.id, format: :csv}
-    assert_response :success
-
-    response_csv = CSV.parse(@response.body)
-    assert_equal 2, response_csv.length
-    header_row = response_csv[0]
-    enrollment_row = response_csv[1]
-    assert_equal @enrollment.email, enrollment_row[header_row.find_index('Email')]
-  end
-
  test 'facilitators cannot see enrollments in workshops they are not facilitating' do
    sign_in @facilitator
    get :index, params: {workshop_id: @unrelated_workshop.id}