Eager load in test, and backfill the coverage that was hiding

backend/app/controllers/api/v1/aws_ses_controller.rb

@@ -1,8 +1,8 @@
 module Api
   module V1
     class AwsSesController < ApplicationController
-      skip_authorize_resource only: [:mail_it, :notification]
-      skip_before_action :authenticate_user!, only: [:mail_it, :notification]
+      skip_authorize_resource only: [:notification]
+      skip_before_action :authenticate_user!, only: [:notification]
 
       def notification
         message_type = request.headers["x-amz-sns-message-type"]

backend/app/controllers/api/v1/comments_controller.rb

@@ -5,7 +5,13 @@ class CommentsController < ApplicationController
       skip_before_action :authenticate_user!, only: [:index]
 
       def index
-        render json: @comments.where(:id.in => params[:ids]).order_by(created_at: :asc)
+        # CommentSerializer renders each comment's reactions, so without eager loading
+        # this is one extra query per comment. PostsController#index already does the
+        # same for its own associations.
+        render json: @comments
+          .where(:id.in => params[:ids])
+          .includes(:reactions)
+          .order_by(created_at: :asc)
       end
 
       def show
@@ -16,7 +22,7 @@ def create
         @comment.encrypted_user_id = current_user.encrypted_id
 
         if @comment.save
-          UpdatePostCountersJob.perform_async(parent_id: create_params[:post_id], parent_type: "Post")
+          UpdatePostCountersJob.perform_async("parent_id" => create_params[:post_id], "parent_type" => "Post")
 
           unless @comment.encrypted_user_id == @comment.post.encrypted_user_id
             Notification.create(

backend/app/controllers/api/v1/reactions_controller.rb

@@ -15,8 +15,8 @@ def destroy
         authorize! :destroy, reaction
 
         if reaction.destroy
-          UpdatePostCountersJob.perform_async(parent_id: reaction_params[:reactable_id],
-            parent_type: reaction_params[:reactable_type])
+          UpdatePostCountersJob.perform_async("parent_id" => reaction_params[:reactable_id],
+            "parent_type" => reaction_params[:reactable_type])
 
           head :no_content
         else
@@ -32,8 +32,8 @@ def react(method_name)
         authorize! method_name, reaction
 
         if reaction.save
-          UpdatePostCountersJob.perform_async(parent_id: reaction_params[:reactable_id],
-            parent_type: reaction_params[:reactable_type])
+          UpdatePostCountersJob.perform_async("parent_id" => reaction_params[:reactable_id],
+            "parent_type" => reaction_params[:reactable_type])
 
           unless reaction.encrypted_user_id == reaction.reactable.encrypted_user_id
             Notification.create(

backend/app/jobs/email_reject_dispatcher.rb

@@ -10,7 +10,7 @@ def perform(raw_post)
       if test_case_type == "Bounce"
         emails = body.dig("mail", "destination") || []
 
-        {bounce: emails}
+        {"bounce" => emails}
       elsif message_raw
         generate_recipients(message_raw)
       end
@@ -26,6 +26,6 @@ def generate_recipients(message_raw)
     emails = message.dig("mail", "destination") || []
     rejected_type = message["notificationType"].downcase
 
-    {rejected_type.to_sym => emails}
+    {rejected_type => emails}
   end
 end

backend/app/jobs/merge_trackables/trackable_usages.rb

@@ -18,9 +18,10 @@ def perform(trackable_type, parent_id, rest_ids)
 
         tr_usage.destroy
       else
+        # `belongs_to :trackable, counter_cache: true` already moves the count from the
+        # duplicate to the parent when the foreign key changes; incrementing here as well
+        # counted every merged usage twice.
         tr_usage.update(trackable_id: parent.id)
-
-        parent.increment!(:trackable_usages_count)
       end
     end
 

backend/app/jobs/update_checkin_reminders.rb

@@ -11,7 +11,11 @@ def perform(profile_id)
 
     Sidekiq::ScheduledSet.new.find_job(profile.reminder_job_id)&.delete
 
-    job_id = CheckinReminderJob.perform_in(get_reminder_time(profile).minutes, profile_id, profile.checkin_reminder_at)
+    # Sidekiq only accepts native JSON types as job arguments, and checkin_reminder_at
+    # is a datetime column. ProfilesController#schedule_reminder does the same.
+    job_id = CheckinReminderJob.perform_in(
+      get_reminder_time(profile).minutes, profile_id, profile.checkin_reminder_at.iso8601
+    )
     profile.update_column(:reminder_job_id, job_id)
   end
 

backend/config/environments/test.rb

@@ -11,11 +11,19 @@
   # While tests run files are not watched, reloading is not necessary.
   config.enable_reloading = false
 
-  # Eager loading loads your entire application. When running a single test locally,
-  # this is usually not necessary, and can slow down your test suite. However, it's
-  # recommended that you enable it in continuous integration systems to ensure eager
-  # loading is working properly before deploying your code.
-  config.eager_load = ENV["CI"].present?
+  # Eager loading loads your entire application. The Rails default here is
+  # `ENV["CI"].present?`, which makes local and CI runs load different amounts of code.
+  # Two things went wrong with that:
+  #
+  #   1. SimpleCov starts without `track_files`, so it only measures files that were
+  #      actually loaded. Locally that silently excluded 68 app files the suite never
+  #      touches, reporting 95.54% against a denominator defined by the tests
+  #      themselves; CI eager-loaded those same files and reported 86.35%.
+  #   2. Eager loading is what catches autoload and NameError breakage. Gating it on CI
+  #      meant that whole class of bug could only ever fail on CI, never locally.
+  #
+  # Measured cost of always eager loading: about 0.5s on boot (2.7s -> 3.2s).
+  config.eager_load = true
 
   # Configure public file server for tests with Cache-Control for performance.
   config.public_file_server.enabled = true

backend/spec/controllers/api/v1/aws_ses_controller_spec.rb

@@ -0,0 +1,48 @@
+require "rails_helper"
+require "sidekiq/testing"
+
+# SES posts here over SNS. The message type arrives in a header, and the body is read
+# raw rather than through strong parameters.
+RSpec.describe Api::V1::AwsSesController do
+  around do |example|
+    Sidekiq::Testing.fake! do
+      EmailRejectDispatcher.clear
+      example.run
+    end
+  end
+
+  describe "notification" do
+    it "hands a delivery notification to the dispatcher, unauthenticated" do
+      request.headers["x-amz-sns-message-type"] = "Notification"
+      request.headers["CONTENT_TYPE"] = "application/json"
+
+      post :notification, body: {"notificationType" => "Bounce"}.to_json
+
+      expect(response).to have_http_status :ok
+      expect(EmailRejectDispatcher.jobs.size).to eq 1
+    end
+
+    it "passes the body through untouched, since the dispatcher parses it itself" do
+      raw = {"notificationType" => "Complaint"}.to_json
+      request.headers["x-amz-sns-message-type"] = "Notification"
+      request.headers["CONTENT_TYPE"] = "application/json"
+
+      post :notification, body: raw
+
+      expect(EmailRejectDispatcher.jobs.first["args"]).to eq [raw]
+    end
+
+    it "confirms a subscription by fetching the URL SNS supplies" do
+      request.headers["x-amz-sns-message-type"] = "SubscriptionConfirmation"
+      request.headers["CONTENT_TYPE"] = "application/json"
+      confirm_url = "https://sns.example.com/confirm?token=abc"
+
+      expect(controller).to receive(:open).with(confirm_url)
+
+      post :notification, body: {"SubscribeURL" => confirm_url}.to_json
+
+      expect(response).to have_http_status :ok
+      expect(EmailRejectDispatcher.jobs).to be_empty
+    end
+  end
+end

backend/spec/controllers/api/v1/chart_lists_controller_spec.rb

@@ -0,0 +1,58 @@
+require "rails_helper"
+
+RSpec.describe Api::V1::ChartListsController do
+  let(:user) { create(:user) }
+
+  describe "show" do
+    context "when no user logged-in" do
+      it "returns 302 (redirect to sign-in)" do
+        get :show
+
+        expect(response.status).to eq 302
+      end
+    end
+
+    context "when signed in" do
+      before { sign_in user }
+
+      it "renders every payload section, even with nothing tracked" do
+        get :show
+
+        expect(response).to have_http_status :ok
+        expect(response_body[:chart_list][:id]).to eq 1
+        expect(response_body[:chart_list][:payload].keys).to match_array(
+          %w[tags foods symptoms conditions treatments weathersMeasures harveyBradshawIndices]
+        )
+      end
+
+      it "marks a tag from the most recent check-in as currently tracked" do
+        tag = create(:tag)
+        create(:checkin, user_id: user.id, tag_ids: [tag.id], date: Time.zone.now)
+
+        get :show
+
+        expect(response_body[:chart_list][:payload][:tags]).to include([tag.id, tag.name, true])
+      end
+
+      it "reports the weather measures as untracked when the check-in has no weather" do
+        create(:checkin, user_id: user.id, date: Time.zone.now)
+
+        get :show
+
+        tracked_flags = response_body[:chart_list][:payload][:weathersMeasures].map(&:last)
+        expect(tracked_flags).to all(be false)
+      end
+
+      it "only reports the signed-in user's trackables" do
+        someone_else = create(:user)
+        their_tag = create(:tag)
+        create(:checkin, user_id: someone_else.id, tag_ids: [their_tag.id], date: Time.zone.now)
+
+        get :show
+
+        reported_tag_ids = response_body[:chart_list][:payload][:tags].map(&:first)
+        expect(reported_tag_ids).not_to include their_tag.id
+      end
+    end
+  end
+end

backend/spec/controllers/api/v1/charts_controller_spec.rb

@@ -0,0 +1,42 @@
+require "rails_helper"
+
+RSpec.describe Api::V1::ChartsController do
+  let(:user) { create(:user) }
+  let(:start_at) { 7.days.ago.to_date.to_s }
+  let(:end_at) { Date.current.to_s }
+
+  before { sign_in user }
+
+  describe "show" do
+    it "renders a chart for the requested range" do
+      get :show, params: {start_at: start_at, end_at: end_at}
+
+      expect(response).to have_http_status :ok
+    end
+
+    it "includes the user's check-ins that fall inside the range" do
+      checkin = create(:checkin, user_id: user.id, date: 2.days.ago)
+
+      get :show, params: {start_at: start_at, end_at: end_at}
+
+      expect(response.body).to include checkin.id.to_s
+    end
+
+    it "excludes check-ins outside the range" do
+      outside = create(:checkin, user_id: user.id, date: 90.days.ago)
+
+      get :show, params: {start_at: start_at, end_at: end_at}
+
+      expect(response.body).not_to include outside.id.to_s
+    end
+
+    context "when the range is missing" do
+      it "returns 422 with the validation errors" do
+        get :show
+
+        expect(response).to have_http_status :unprocessable_entity
+        expect(response_body[:errors].keys).to include "start_at", "end_at"
+      end
+    end
+  end
+end

backend/spec/controllers/api/v1/charts_pattern_controller_spec.rb

@@ -0,0 +1,80 @@
+require "rails_helper"
+
+RSpec.describe Api::V1::ChartsPatternController do
+  let(:user) { create(:user) }
+  let!(:pattern) do
+    create(:pattern,
+      encrypted_user_id: user.encrypted_id,
+      includes: [{category: "treatments"}])
+  end
+
+  let(:base_params) do
+    {
+      start_at: 7.days.ago.to_date.to_s,
+      end_at: Date.current.to_s,
+      offset: 0,
+      pattern_ids: [pattern.id.to_s]
+    }
+  end
+
+  before { sign_in user }
+
+  describe "index" do
+    it "returns chart data for each requested pattern" do
+      get :index, params: base_params
+
+      expect(response).to have_http_status :ok
+      expect(response_body[:charts_pattern].size).to eq 1
+      expect(response_body[:charts_pattern].first[:pattern_name]).to eq pattern.name
+    end
+
+    it "advertises the available colours in the metadata" do
+      get :index, params: base_params
+
+      expect(response_body[:meta][:color_ids]).to eq Flaredown::Colorable::IDS
+    end
+
+    it "returns nothing when no pattern ids are given" do
+      get :index, params: base_params.except(:pattern_ids)
+
+      expect(response_body[:charts_pattern]).to be_empty
+    end
+
+    it "is readable without signing in" do
+      sign_out user
+
+      get :index, params: base_params
+
+      expect(response).to have_http_status :ok
+    end
+
+    # The offset widens the window backwards, and forwards too unless the range already
+    # ends today -- a request ending today is left alone so it does not ask for the
+    # future.
+    it "widens the window backwards by the offset" do
+      expect(ChartsPattern).to receive(:new)
+        .with(hash_including(start_at: 10.days.ago.to_date.to_s))
+        .and_call_original
+
+      get :index, params: base_params.merge(start_at: 7.days.ago.to_date.to_s, offset: 3)
+    end
+
+    it "leaves an end date of today unshifted" do
+      expect(ChartsPattern).to receive(:new)
+        .with(hash_including(end_at: Date.current.to_s))
+        .and_call_original
+
+      get :index, params: base_params.merge(offset: 3)
+    end
+
+    it "shifts an end date in the past forwards by the offset" do
+      end_date = 2.days.ago.to_date
+
+      expect(ChartsPattern).to receive(:new)
+        .with(hash_including(end_at: (end_date + 3.days).to_s))
+        .and_call_original
+
+      get :index, params: base_params.merge(end_at: end_date.to_s, offset: 3)
+    end
+  end
+end