AO3-6232 Include IP, referer, skin in support tickets (#4869)

* AO3-6232 Include IP, referer, skin in support tickets

* Whoopses

* Extract current_skin helper

* Cleanup

* Revert "Cleanup"

This reverts commit 5f256af.

* Review feedback

* Better handling for empty referer

* Fix more tests

* Undo field rename

app/controllers/feedbacks_controller.rb

@@ -19,6 +19,8 @@ def create
     @feedback.rollout = @feedback.rollout_string
     @feedback.user_agent = request.env["HTTP_USER_AGENT"]
     @feedback.ip_address = request.remote_ip
+    @feedback.referer = request.referer if request.referer && ArchiveConfig.PERMITTED_HOSTS.include?(URI(request.referer).host)
+    @feedback.site_skin = helpers.current_skin
     if @feedback.save
       @feedback.email_and_send
       flash[:notice] = t("successfully_sent",
@@ -42,5 +44,4 @@ def feedback_params
       :comment, :email, :summary, :username, :language
     )
   end
-
 end

app/helpers/skins_helper.rb

@@ -14,45 +14,41 @@ def skin_preview_display(skin)
     end
   end
 
-  def skin_tag
-    skin = nil
-
-    if params[:site_skin]
-      skin ||= Skin.approved_or_owned_by.usable.find_by(id: params[:site_skin])
-    end
-
-    if (logged_in? || logged_in_as_admin?) && session[:site_skin]
-      skin ||= Skin.approved_or_owned_by.usable.find_by(id: session[:site_skin])
-    end
-
-    skin_id = if skin.nil?
-                current_user&.preference&.skin_id || AdminSetting.default_skin_id
-              else
-                skin.id
-              end
-
-    return "" if skin_id.nil?
+  # Fetches the current skin. This is determined by the following
+  # 1. Skin ID set by request parameter
+  # 2. Skin ID set in the current session (if someone, a user or admin, is logged in)
+  # 3. Current user's skin preference
+  # 4. The default skin (as set by the active AdminSetting)
+  def current_skin
+    skin = Skin.approved_or_owned_by.usable.find_by(id: params[:site_skin]) if params[:site_skin]
+    skin ||= Skin.approved_or_owned_by.usable.find_by(id: session[:site_skin]) if (logged_in? || logged_in_as_admin?) && session[:site_skin]
+    skin ||= current_user&.preference&.skin
+    skin || AdminSetting.default_skin
+  end
 
+  def skin_tag
     roles = if logged_in_as_admin?
               Skin::DEFAULT_ROLES_TO_INCLUDE + ["admin"]
             else
               Skin::DEFAULT_ROLES_TO_INCLUDE
             end
 
-    # We include the version information for both the skin_id and the
+    skin = current_skin
+    return "" unless skin
+
+    # We include the version information for both the skin's id and the
     # AdminSetting.default_skin_id because the default skin is used in skins of
     # type "user", so we need to regenerate the cache block when it's modified.
     #
     # We also include the default_skin_id in the version number so that we
     # regenerate the cache block when an admin updates the current default
     # skin.
     Rails.cache.fetch(
-      [:v1, :site_skin, skin_id, logged_in_as_admin?],
-      version: [skin_cache_version(skin_id),
+      [:v1, :site_skin, skin.id, logged_in_as_admin?],
+      version: [skin_cache_version(skin.id),
                 AdminSetting.default_skin_id,
                 skin_cache_version(AdminSetting.default_skin_id)]
     ) do
-      skin ||= Skin.find(skin_id)
       skin.get_style(roles)
     end
   end

app/models/feedback.rb

@@ -1,8 +1,8 @@
 # Class which holds feedback sent to the archive administrators about the archive as a whole
 class Feedback < ApplicationRecord
-  attr_accessor :ip_address
+  attr_accessor :ip_address, :referer, :site_skin
 
-  # note -- this has NOTHING to do with the Comment class!
+  # NOTE: this has NOTHING to do with the Comment class!
   # This is just the name of the text field in the Feedback
   # class which holds the user's comments.
   validates_presence_of :comment
@@ -60,7 +60,8 @@ def rollout_string
   end
 
   def send_report
-    return unless %w(staging production).include?(Rails.env)
+    return unless zoho_enabled?
+
     reporter = SupportReporter.new(
       title: summary,
       description: comment,
@@ -69,8 +70,17 @@ def send_report
       username: username,
       user_agent: user_agent,
       site_revision: ArchiveConfig.REVISION.to_s,
-      rollout: rollout
+      rollout: rollout,
+      ip_address: ip_address,
+      referer: referer,
+      site_skin: site_skin
     )
     reporter.send_report!
   end
+
+  private
+
+  def zoho_enabled?
+    %w[staging production].include?(Rails.env)
+  end
 end

app/models/feedback_reporters/abuse_reporter.rb

@@ -1,6 +1,4 @@
 class AbuseReporter < FeedbackReporter
-  attr_accessor :ip_address
-
   def report_attributes
     super.deep_merge(
       "departmentId" => department_id,

app/models/feedback_reporters/feedback_reporter.rb

@@ -13,7 +13,8 @@ class FeedbackReporter
                 :language,
                 :category,
                 :username,
-                :url
+                :url,
+                :ip_address
 
   def initialize(attrs = {})
     attrs.each_pair do |key, val|

app/models/feedback_reporters/support_reporter.rb

@@ -1,5 +1,5 @@
 class SupportReporter < FeedbackReporter
-  attr_accessor :user_agent, :site_revision, :rollout
+  attr_accessor :user_agent, :referer, :rollout, :site_revision, :site_skin
 
   def report_attributes
     super.deep_merge(
@@ -13,10 +13,17 @@ def report_attributes
   private
 
   def custom_zoho_fields
+    # To avoid issues where Zoho ticket creation silently fails, only grab the first
+    # 255 characters of the referer URL. That may miss some complex search queries,
+    # but still keep enough to be useful most of the time.
+    truncated_referer = referer.present? ? referer[0..254] : "Unknown URL"
     {
       "cf_archive_version" => site_revision.presence || "Unknown site revision",
       "cf_rollout" => rollout.presence || "Unknown",
-      "cf_user_agent" => user_agent.presence || "Unknown user agent"
+      "cf_user_agent" => user_agent.presence || "Unknown user agent",
+      "cf_ip" => ip_address.presence || "Unknown IP",
+      "cf_referer" => truncated_referer,
+      "cf_site_skin" => site_skin&.public ? site_skin.title : "Custom skin"
     }
   end
 

app/views/feedbacks/new.html.erb

@@ -42,9 +42,6 @@
   <%= form_for(@feedback, html: {class: "post feedback"}) do |f| %>
     <fieldset>
       <legend><%= t(".form.legend.contact_info") %></legend>
-      <p class="notice">
-        <%= t(".form.ip") %>
-      </p>
       <dl>
         <dt><%= f.label :username, t(".form.name.label") %></dt>
         <dd>

config/locales/views/en.yml

@@ -600,7 +600,6 @@ en:
           label: Your question or problem (required)
         email:
           label: Your email (required)
-        ip: Our spam filter does collect IP addresses, but we never see them.
         language:
           label: Select language (required)
         legend:

spec/controllers/feedbacks_controller_spec.rb

@@ -0,0 +1,121 @@
+# frozen_string_literal: true
+
+require "spec_helper"
+
+describe FeedbacksController do
+  include LoginMacros
+
+  describe "POST #create" do
+    let(:mock_zoho) { instance_double(ZohoResourceClient) }
+
+    let(:default_parameters) do
+      {
+        feedback: {
+          comment: "Hello",
+          email: "[email protected]",
+          summary: "Summary",
+          language: "en"
+        }
+      }
+    end
+
+    before do
+      allow_any_instance_of(Feedback).to receive(:zoho_enabled?).and_return(true)
+      allow(mock_zoho).to receive(:retrieve_contact_id)
+      allow_any_instance_of(FeedbackReporter).to receive(:zoho_resource_client).and_return(mock_zoho)
+    end
+
+    context "when accessed by a logged-in user" do
+      let(:user) { create(:user) }
+
+      before do
+        fake_login_known_user(user)
+      end
+
+      context "when the user has no skin set" do
+        before do
+          admin_setting = AdminSetting.default
+          admin_setting.default_skin = Skin.default
+          admin_setting.save(validate: false)
+        end
+
+        it "sets the skin title in the Zoho ticket" do
+          expect(mock_zoho).to receive(:create_ticket).with(ticket_attributes: include(
+            "cf" => include(
+              "cf_site_skin" => Skin.default.title
+            )
+          ))
+          post :create, params: default_parameters
+        end
+      end
+
+      context "when the user has a public non-default skin set" do
+        let(:skin) { create(:skin, :public) }
+
+        before do
+          user.preference.update!(skin: skin)
+        end
+
+        it "sets the skin title in the Zoho ticket" do
+          expect(mock_zoho).to receive(:create_ticket).with(ticket_attributes: include(
+            "cf" => include(
+              "cf_site_skin" => skin.title
+            )
+          ))
+          post :create, params: default_parameters
+        end
+      end
+
+      context "when the user has a private skin set" do
+        let(:skin) { create(:skin, author: user) }
+
+        before do
+          user.preference.update!(skin: skin)
+        end
+
+        it "sets the expected fields in the support ticket" do
+          expect(mock_zoho).to receive(:create_ticket).with(ticket_attributes: include(
+            "cf" => include(
+              "cf_site_skin" => "Custom skin"
+            )
+          ))
+          post :create, params: default_parameters
+        end
+      end
+    end
+
+    context "when accessed by a guest" do
+      context "when the referer is on the Archive" do
+        before do
+          request.env["HTTP_REFERER"] = "https://archiveofourown.org/works/1"
+        end
+
+        it "sets the referer in the Zoho ticket" do
+          expect(mock_zoho).to receive(:create_ticket).with(ticket_attributes: include(
+            "cf" => include(
+              "cf_referer" => "https://archiveofourown.org/works/1",
+              "cf_ip" => "0.0.0.0"
+            )
+          ))
+          post :create, params: default_parameters
+        end
+      end
+
+      context "when the referer is elsewhere" do
+        before do
+          request.env["HTTP_REFERER"] = "https://example.com/works/1"
+        end
+
+        it "does not set the referer in the Zoho ticket" do
+          expect(mock_zoho).to receive(:create_ticket).with(ticket_attributes: include(
+            "cf" => include(
+              "cf_referer" => "Unknown URL",
+              "cf_ip" => "0.0.0.0"
+            )
+          ))
+          post :create, params: default_parameters
+        end
+      end
+    end
+  end
+end

spec/helpers/skins_helper_spec.rb

@@ -0,0 +1,77 @@
+# frozen_string_literal: true
+
+require "spec_helper"
+
+describe SkinsHelper do
+  describe "#current_skin" do
+    before do
+      allow(helper).to receive(:current_user)
+      allow(helper).to receive(:logged_in_as_admin?).and_return(false)
+      allow(helper).to receive(:logged_in?).and_return(false)
+      admin_setting = AdminSetting.default
+      admin_setting.default_skin = Skin.default
+      admin_setting.save(validate: false)
+    end
+
+    context "when the parameters include a skin id" do
+      before do
+        params[:site_skin] = skin.id
+      end
+
+      context "when the skin is applied" do
+        let(:skin) { create(:skin, :public) }
+
+        it "returns the skin matching the parameter" do
+          expect(helper.current_skin).to eq(skin)
+        end
+      end
+
+      context "when the skin is not applied" do
+        let(:skin) { create(:skin) }
+
+        it "falls back to other options" do
+          expect(helper.current_skin).to eq(Skin.default)
+        end
+      end
+    end
+
+    context "when the current user has a skin set for the session" do
+      before do
+        allow(helper).to receive(:current_user).and_return(create(:user))
+        allow(helper).to receive(:logged_in?).and_return(true)
+        session[:site_skin] = skin.id
+      end
+
+      context "when the skin is applied" do
+        let(:skin) { create(:skin, :public) }
+
+        it "returns the skin matching the session attribute" do
+          expect(helper.current_skin).to eq(skin)
+        end
+      end
+
+      context "when the skin is not applied" do
+        # Non-public skin with a different author
+        let(:skin) { create(:skin) }
+
+        it "falls back to other options" do
+          expect(helper.current_skin).to eq(Skin.default)
+        end
+      end
+    end
+
+    context "when the current user has a skin preference set" do
+      let(:skin) { create(:skin) }
+      let(:user) { skin.author }
+
+      before do
+        user.preference.update!(skin: skin)
+        allow(helper).to receive(:current_user).and_return(user)
+      end
+
+      it "returns the preferred skin" do
+        expect(helper.current_skin).to eq(skin)
+      end
+    end
+  end
+end