Merge branch 'main' into lois/1000-return-remix-identifier-with-lesson

loiswells97 · web-flow · commit 1c2603a494da · 2025-11-18T12:34:21.000Z
diff --git a/.clabot b/.clabot
@@ -5,8 +5,10 @@
     "chris.lowis@gofreerange.com",
     "chrisroos",
     "conorriches",
+    "Copilot",
     "create-issue-branch",
     "create-issue-branch[bot]",
+    "cursor",
     "danhalson",
     "dependabot[bot]",
     "fspeirs",
@@ -16,17 +18,17 @@
     "jamiebenstead",
     "jrmhaig",
     "KristinaDudnyk",
-    "lpmi-13",
     "loiswells97",
+    "lpmi-13",
     "m-bowley",
     "magdalenajadach",
     "MarcScott",
     "Mohamed-RPF",
-    "pjbRPF",
     "patch0",
     "PetarSimonovic",
+    "pjbRPF",
     "sra405",
     "tuzz"
   ],
   "message": "We require contributors to sign our Contributor License Agreement, and we don't have you on file. In order for us to review and merge your code, please complete [this form](https://form.raspberrypi.org/4873530) and we'll get you added and review your contribution as soon as possible."
-}
+}
diff --git a/.env.example b/.env.example
@@ -47,3 +47,7 @@ ACTIVE_RECORD_ENCRYPTION_DETERMINISTIC_KEY=deterministic-key
 ACTIVE_RECORD_ENCRYPTION_KEY_DERIVATION_SALT=derivation-salt
 
 EDITOR_ENCRYPTION_KEY=a1b2c3d4e5f67890123456789abcdef0123456789abcdef0123456789abcdef0
+
+# The sandbox creds can be found in 1password under "Google Cloud Console: CEfE Sandbox"
+GOOGLE_CLIENT_ID=changeme.apps.googleusercontent.com
+GOOGLE_CLIENT_SECRET=changeme
diff --git a/app/controllers/api/feedback_controller.rb b/app/controllers/api/feedback_controller.rb
@@ -15,7 +15,7 @@ def index
       project_feedback.each do |feedback|
         authorize! :read, feedback
       end
-      @feedback = project_feedback.accessible_by(current_ability)
+      @feedback = project_feedback.accessible_by(current_ability).order(created_at: :asc)
       render :index, formats: [:json], status: :ok
     end
 
@@ -30,6 +30,18 @@ def create
       end
     end
 
+    def set_read
+      feedback = Feedback.find(params[:id])
+      result = Feedback::SetRead.call(feedback: feedback)
+
+      if result.success?
+        @feedback = result[:feedback]
+        render :show, formats: [:json], status: :ok
+      else
+        render json: { error: result[:error] }, status: :unprocessable_entity
+      end
+    end
+
     private
 
     def project
@@ -68,8 +80,8 @@ def feedback_create_params
     end
 
     def url_params
-      permitted_params = params.permit(:project_id)
-      { identifier: permitted_params[:project_id] }
+      permitted_params = params.permit(:project_id, :id)
+      { identifier: permitted_params[:project_id], id: permitted_params[:id] }
     end
 
     def base_params
diff --git a/app/controllers/api/google_auth_controller.rb b/app/controllers/api/google_auth_controller.rb
@@ -0,0 +1,54 @@
+# frozen_string_literal: true
+
+module Api
+  class GoogleAuthController < ApiController
+    TOKEN_EXCHANGE_URL = 'https://oauth2.googleapis.com/token'
+
+    before_action :authorize_user
+    authorize_resource :google_auth, class: false
+
+    def exchange_code
+      response = faraday.post(TOKEN_EXCHANGE_URL, token_exchange_payload)
+      @token_response = JSON.parse(response.body)
+
+      return render(:exchange_code, status: :ok) if response.success?
+
+      render json: { error: response_error_message }, status: :unauthorized
+    rescue JSON::ParserError => e
+      render json: { error: e.message }, status: :bad_gateway
+    rescue Faraday::Error => e
+      render json: { error: e.message }, status: :service_unavailable
+    end
+
+    private
+
+    def faraday
+      Faraday.new do |f|
+        f.request :url_encoded
+        f.options.timeout = 10
+        f.options.open_timeout = 5
+      end
+    end
+
+    def token_exchange_payload
+      payload = google_token_params
+      {
+        code: payload[:code],
+        client_id: ENV.fetch('GOOGLE_CLIENT_ID'),
+        client_secret: ENV.fetch('GOOGLE_CLIENT_SECRET'),
+        redirect_uri: payload[:redirect_uri],
+        grant_type: 'authorization_code'
+      }
+    end
+
+    def response_error_message
+      @token_response['error_description'] || @token_response['error'] || 'Unknown error'
+    end
+
+    def google_token_params
+      params.require(:google_auth).require(:code)
+      params.require(:google_auth).require(:redirect_uri)
+      params.require(:google_auth).permit(:code, :redirect_uri)
+    end
+  end
+end
diff --git a/app/controllers/auth_controller.rb b/app/controllers/auth_controller.rb
@@ -1,10 +1,6 @@
 # frozen_string_literal: true
 
 class AuthController < ApplicationController
-  # def index
-
-  # end
-
   def callback
     Rails.logger.debug { "callback: #{omniauth_params}" }
     # Prevent session fixation.  If the session has been initialized before
diff --git a/app/models/ability.rb b/app/models/ability.rb
@@ -69,6 +69,7 @@ def define_school_owner_abilities(school:)
     can(%i[read create create_batch update destroy], :school_student)
     can(%i[create create_copy], Lesson, school_id: school.id)
     can(%i[read update destroy], Lesson, school_id: school.id, visibility: %w[teachers students public])
+    can(%i[exchange_code], :google_auth)
   end
 
   def define_school_teacher_abilities(user:, school:)
@@ -98,6 +99,7 @@ def define_school_teacher_abilities(user:, school:)
     can(%i[read], Project, remixed_from_id: teacher_project_ids)
     can(%i[show_status unsubmit return complete], SchoolProject, project: { remixed_from_id: teacher_project_ids })
     can(%i[read create], Feedback, school_project: { project: { remixed_from_id: teacher_project_ids } })
+    can(%i[exchange_code], :google_auth)
   end
 
   def define_school_student_abilities(user:, school:)
@@ -113,7 +115,7 @@ def define_school_student_abilities(user:, school:)
     can(%i[read], Lesson, school_id: school.id, visibility: 'students', school_class: { students: { student_id: user.id } })
     can(%i[read create update], Project, school_id: school.id, user_id: user.id, lesson_id: nil, remixed_from_id: visible_lesson_project_ids)
     can(%i[read show_context], Project, lesson: { school_id: school.id, visibility: 'students', school_class: { students: { student_id: user.id } } })
-    can(%i[read], Feedback, school_project: { project: { school_id: school.id, user_id: user.id, lesson_id: nil, remixed_from_id: visible_lesson_project_ids } })
+    can(%i[read set_read], Feedback, school_project: { project: { school_id: school.id, user_id: user.id, lesson_id: nil, remixed_from_id: visible_lesson_project_ids } })
     can(%i[show_finished set_finished show_status unsubmit submit], SchoolProject, project: { user_id: user.id, lesson_id: nil }, school_id: school.id)
   end
 
diff --git a/app/views/api/feedback/_feedback.json.jbuilder b/app/views/api/feedback/_feedback.json.jbuilder
@@ -7,5 +7,6 @@ json.call(
   :user_id,
   :content,
   :created_at,
-  :updated_at
+  :updated_at,
+  :read_at
 )
diff --git a/app/views/api/google_auth/exchange_code.json.jbuilder b/app/views/api/google_auth/exchange_code.json.jbuilder
@@ -0,0 +1,9 @@
+# frozen_string_literal: true
+
+json.call(
+  @token_response,
+  'access_token',
+  'expires_in',
+  'token_type',
+  'scope'
+)
diff --git a/config/routes.rb b/config/routes.rb
@@ -48,7 +48,9 @@
       end
       resources :remixes, only: %i[index], controller: 'projects/remixes'
       resource :images, only: %i[show create], controller: 'projects/images'
-      resources :feedback, only: %i[index create], controller: 'feedback'
+      resources :feedback, only: %i[index create], controller: 'feedback' do
+        put :read, on: :member, to: 'feedback#set_read'
+      end
     end
 
     resource :project_errors, only: %i[create]
@@ -79,6 +81,8 @@
     end
 
     resources :user_jobs, only: %i[index show]
+
+    post '/google/auth/exchange-code', to: 'google_auth#exchange_code', defaults: { format: :json }
   end
 
   resource :github_webhooks, only: :create, defaults: { formats: :json }
diff --git a/db/migrate/20251106170425_add_read_at_to_feedback.rb b/db/migrate/20251106170425_add_read_at_to_feedback.rb
@@ -0,0 +1,5 @@
+class AddReadAtToFeedback < ActiveRecord::Migration[7.2]
+  def change
+    add_column :feedback, :read_at, :datetime
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
diff --git a/lib/concepts/feedback/set_read.rb b/lib/concepts/feedback/set_read.rb
@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+class Feedback
+  class SetRead
+    class << self
+      def call(feedback:)
+        response = OperationResponse.new
+        response[:feedback] = feedback
+        response[:feedback].read_at = Time.current
+        response[:feedback].save!
+        response
+      rescue StandardError => e
+        Sentry.capture_exception(e)
+        response[:error] = e.message
+        response
+      end
+    end
+  end
+end
diff --git a/spec/concepts/feedback/set_read_at_spec.rb b/spec/concepts/feedback/set_read_at_spec.rb
@@ -0,0 +1,53 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe Feedback::SetRead, type: :unit do
+  let(:feedback) { create(:feedback) }
+
+  describe '.call' do
+    context 'when set_read is successful' do
+      it 'returns a successful operation response' do
+        response = described_class.call(feedback: feedback)
+        expect(response.success?).to be(true)
+      end
+
+      it 'returns the updated feedback' do
+        response = described_class.call(feedback: feedback)
+        expect(response[:feedback]).to be_a(Feedback)
+      end
+
+      it 'returns read_at' do
+        response = described_class.call(feedback: feedback)
+        expect(response[:feedback].read_at).to be_present
+      end
+
+      it 'returns read_at as a timestamp' do
+        response = described_class.call(feedback: feedback)
+        expect(response[:feedback].read_at).to be_a(ActiveSupport::TimeWithZone)
+      end
+    end
+
+    context 'when set_read fails' do
+      before do
+        allow(feedback).to receive(:save!).and_raise(StandardError, 'Some API error')
+      end
+
+      it 'returns a failed operation response' do
+        response = described_class.call(feedback: feedback)
+        expect(response.success?).to be(false)
+      end
+
+      it 'does not persist read_at' do
+        described_class.call(feedback: feedback)
+        feedback.reload
+        expect(feedback.read_at).to be_nil
+      end
+
+      it 'includes the correct error response' do
+        response = described_class.call(feedback: feedback)
+        expect(response[:error]).to eq('Some API error')
+      end
+    end
+  end
+end
diff --git a/spec/features/feedback/setting_read_at_spec.rb b/spec/features/feedback/setting_read_at_spec.rb
@@ -0,0 +1,87 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe 'Set read_at on feedback requests', type: :request do
+  let(:headers) { { Authorization: UserProfileMock::TOKEN } }
+  let(:school) { create(:school) }
+  let(:student) { create(:student, school:) }
+  let(:teacher) { create(:teacher, school:) }
+  let(:school_class) { create(:school_class, teacher_ids: [teacher.id], school:) }
+  let(:class_student) { create(:class_student, school_class:, student_id: student.id) }
+  let(:lesson) { create(:lesson, school:, school_class:, user_id: teacher.id, visibility: 'students') }
+  let(:teacher_project) { create(:project, user_id: teacher.id, school:, lesson:) }
+  let(:student_project) { create(:project, user_id: class_student.student_id, school:, parent: teacher_project) }
+  let!(:feedback) { create(:feedback, school_project: student_project.school_project, user_id: teacher.id, content: 'Excellent work!') }
+  let(:feedback_json) do
+    {
+      id: feedback.id,
+      school_project_id: feedback.school_project_id,
+      user_id: feedback.user_id,
+      content: feedback.content,
+      created_at: feedback.created_at,
+      updated_at: feedback.updated_at,
+      read_at: feedback.read_at
+    }.to_json
+  end
+
+  context 'when logged in as the class teacher' do
+    before do
+      authenticated_in_hydra_as(teacher)
+      put("/api/projects/#{student_project.identifier}/feedback/#{feedback.id}/read", headers: headers)
+      feedback.reload
+    end
+
+    it 'returns forbidden response' do
+      expect(response).to have_http_status(:forbidden)
+    end
+
+    it 'does not set the read_at on feedback' do
+      expect(feedback.read_at).to be_nil
+    end
+  end
+
+  context 'when logged in as the student' do
+    before do
+      authenticated_in_hydra_as(student)
+    end
+
+    context 'when feedback exists' do
+      before do
+        put("/api/projects/#{student_project.identifier}/feedback/#{feedback.id}/read", headers: headers)
+        feedback.reload
+      end
+
+      it 'returns ok response' do
+        expect(response).to have_http_status(:ok)
+      end
+
+      it 'returns the feedback json' do
+        expect(response.body).to eq(feedback_json)
+      end
+
+      it 'does set the read_at on feedback' do
+        expect(feedback.read_at).to be_present
+      end
+
+      it 'sets read_at to be a time' do
+        expect(feedback.read_at).to be_a(ActiveSupport::TimeWithZone)
+      end
+
+      it 'sets read_at to a recent time' do
+        expect(feedback.read_at).to be_within(5.seconds).of(Time.current)
+      end
+    end
+
+    context 'when feedback does not exist' do
+      before do
+        put("/api/projects/#{student_project.identifier}/feedback/does-not-exist/read", headers: headers)
+        feedback.reload
+      end
+
+      it 'returns not found response' do
+        expect(response).to have_http_status(:not_found)
+      end
+    end
+  end
+end
diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb
diff --git a/spec/requests/api/google_auth_controller_spec.rb b/spec/requests/api/google_auth_controller_spec.rb

Original file line number	Diff line number	Diff line change
`@@ -7,5 +7,6 @@ json.call(`
`7`	`7`	`:user_id,`
`8`	`8`	`:content,`
`9`	`9`	`:created_at,`
`10`		`- :updated_at`
	`10`	`+ :updated_at,`
	`11`	`+ :read_at`
`11`	`12`	`)`