diff --git a/Gemfile b/Gemfile index d00583a87c836..ee891ad7a6242 100644 --- 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 index f12c0510e143b..d68cb99bdb24d 100644 --- 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 index 65e76af53b292..e7c02f1c5f73f 100644 --- 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 index 05bc412e4bf07..5e6c3e1c2d054 100644 --- 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 index f34627f583cab..3514c6975659e 100644 --- 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 index 76f9b646b3080..cb10c88cbf7ac 100644 --- 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 index f724a7e87dbf6..e045e5dcb9200 100644 --- 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//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//using_text_mode diff --git a/dashboard/app/controllers/script_levels_controller.rb b/dashboard/app/controllers/script_levels_controller.rb index 8dbf7a2640b81..941c3febe829b 100644 --- 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 index 7ad7e3360732e..44af852b0ac6e 100644 --- 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 index b3ca451d42078..006f53f7093c3 100644 --- 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 index 553f901af5940..05b97456d5042 100644 --- 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 index b8e1b4f2caf90..fed44ac820df5 100644 --- 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 index 8affb14261303..48afabe62c42d 100644 --- 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 index e967c974906c3..78fff0f08bede 100644 --- 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 index 289245be84576..8d7212c423bc7 100644 --- 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 index 79a942f15059a..0c11a67d23bd9 100644 --- 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} diff --git a/dashboard/test/controllers/api/v1/teacher_feedbacks_controller_test.rb b/dashboard/test/controllers/api/v1/teacher_feedbacks_controller_test.rb index 5cb37b6e7e9ce..c4d9b1f69f447 100644 --- a/dashboard/test/controllers/api/v1/teacher_feedbacks_controller_test.rb +++ b/dashboard/test/controllers/api/v1/teacher_feedbacks_controller_test.rb @@ -137,7 +137,7 @@ class Api::V1::TeacherFeedbacksControllerTest < ActionDispatch::IntegrationTest sign_in @student get "#{API}/count" - assert_equal "0", response.body + assert_equal "0", formatted_response end test 'count is accurate when feedback is available' do @@ -147,7 +147,8 @@ class Api::V1::TeacherFeedbacksControllerTest < ActionDispatch::IntegrationTest sign_in @student get "#{API}/count" - assert_equal "1", response.body + + assert_equal "1", formatted_response teacher_sign_in_and_give_feedback(@teacher, @student, @level, @script_level, COMMENT2, PERFORMANCE2) sign_out @teacher @@ -155,7 +156,7 @@ class Api::V1::TeacherFeedbacksControllerTest < ActionDispatch::IntegrationTest sign_in @student get "#{API}/count" - assert_equal "2", response.body + assert_equal "2", formatted_response end test 'count does not include already seen feedback' do @@ -166,7 +167,7 @@ class Api::V1::TeacherFeedbacksControllerTest < ActionDispatch::IntegrationTest sign_in @student get "#{API}/count" - assert_equal "1", response.body + assert_equal "1", formatted_response TeacherFeedback.last.update_attribute( :seen_on_feedback_page_at, @@ -174,7 +175,7 @@ class Api::V1::TeacherFeedbacksControllerTest < ActionDispatch::IntegrationTest ) get "#{API}/count" - assert_equal "0", response.body + assert_equal "0", formatted_response end test 'count does not include feedback from a not authorized teacher' do @@ -184,7 +185,7 @@ class Api::V1::TeacherFeedbacksControllerTest < ActionDispatch::IntegrationTest sign_in @student get "#{API}/count" - assert_equal "0", response.body + assert_equal "0", formatted_response end test 'bad request when student_id not provided - get_feedbacks' do @@ -295,6 +296,10 @@ def parsed_response JSON.parse(@response.body) end + def formatted_response + @response.body.delete!('\\"') + end + # Sign in as teacher and leave feedback for student on level. # Assert that the feedback request was successful def teacher_sign_in_and_give_feedback(teacher, student, level, script_level, comment, performance)