Responding to code review comments

Renaming "referrer" keys and values to "referer" because it's spelled
wrong.

Renamed `external_spam_check_result` and `#external_verdict` to
`spamcheck_result` and `#spamcheck_verdict` respectively.

Adding tests for protobuf object creation methods in
`Spamcheck::Client`.

app/services/spam/akismet_service.rb

@@ -20,7 +20,7 @@ module Spam
         created_at: DateTime.current,
         author: owner_name,
         author_email: owner_email,
-        referrer: options[:referrer]
+        referer: options[:referer]
       }
 
       begin

app/services/spam/spam_action_service.rb

@@ -53,7 +53,7 @@ module Spam
       if request
         options[:ip_address] = request.env['action_dispatch.remote_ip'].to_s
         options[:user_agent] = request.env['HTTP_USER_AGENT']
-        options[:referrer] = request.env['HTTP_REFERRER']
+        options[:referer] = request.env['HTTP_REFERER']
       else
         # TODO: This code is never used, because we do not perform a verification if there is not a
         #   request.  Why? Should it be deleted? Or should we check even if there is no request?
@@ -125,6 +125,7 @@ module Spam
           create_spam_log(api)
         when BLOCK_USER
           # TODO: improve BLOCK_USER handling, non-existent until now
+          # https://gitlab.com/gitlab-org/gitlab/-/issues/329666
           target.spam! unless target.allow_possible_spam?
           create_spam_log(api)
         when ALLOW

app/services/spam/spam_verdict_service.rb

@@ -14,16 +14,16 @@ module Spam
     end
 
     def execute
-      external_spam_check_result = nil
+      spamcheck_result = nil
 
       external_spam_check_round_trip_time = Benchmark.realtime do
-        external_spam_check_result = external_verdict
+        spamcheck_result = spamcheck_verdict
       end
 
       akismet_result = akismet_verdict
 
       # filter out anything we don't recognise, including nils.
-      valid_results = [external_spam_check_result, akismet_result].compact.select { |r| SUPPORTED_VERDICTS.key?(r) }
+      valid_results = [spamcheck_result, akismet_result].compact.select { |r| SUPPORTED_VERDICTS.key?(r) }
 
       # Treat nils - such as service unavailable - as ALLOW
       return ALLOW unless valid_results.any?
@@ -33,7 +33,7 @@ module Spam
 
       logger.info(source: 'spam_verdict_service.rb',
                   akismet_verdict: akismet_verdict,
-                  spam_check_verdict: external_verdict,
+                  spam_check_verdict: spamcheck_result,
                   spam_check_rtt: external_spam_check_round_trip_time.real,
                   final_verdict: final_verdict,
                   username: user.username,
@@ -57,14 +57,14 @@ module Spam
       end
     end
 
-    def external_verdict
+    def spamcheck_verdict
       return unless Gitlab::CurrentSettings.spam_check_endpoint_enabled
 
       begin
         result, _error = spamcheck_client.issue_spam?(spam_issue: target, user: user, context: context)
         return unless result
 
-        # @TODO log if error is not nil
+        # @TODO log if error is not nil https://gitlab.com/gitlab-org/gitlab/-/issues/329545
 
         if Gitlab::Recaptcha.enabled? && result == CONDITIONAL_ALLOW
           ALLOW

lib/gitlab/spamcheck/client.rb

@@ -40,8 +40,8 @@ module Gitlab
       def issue_spam?(spam_issue:, user:, context: {})
         issue = build_issue_protobuf(issue: spam_issue, user: user, context: context)
 
-        response = @stub.check_for_spam_issue(issue, metadata: 
-                                              {'authorization' => ENV['SPAMCHECK_APIKEY']})
+        response = @stub.check_for_spam_issue(issue, metadata:
+                                              { 'authorization' => ENV['SPAMCHECK_APIKEY'] })
         verdict = convert_verdict_to_gitlab_constant(response.verdict)
         [verdict, response.error]
       end
@@ -52,7 +52,7 @@ module Gitlab
         VERDICT_MAPPING.fetch(::Spamcheck::SpamVerdict::Verdict.resolve(verdict), verdict)
       end
 
-      def build_issue_protobuf(issue:, user:, context: )
+      def build_issue_protobuf(issue:, user:, context:)
         issue_pb = ::Spamcheck::Issue.new
         issue_pb.title = issue.spam_title || ''
         issue_pb.description = issue.spam_description || ''

spec/lib/gitlab/spamcheck/client_spec.rb

@@ -6,10 +6,10 @@ RSpec.describe Gitlab::Spamcheck::Client do
   include_context 'includes Spam constants'
 
   let(:endpoint) { 'grpc://grpc.test.url' }
-  let(:user) { create(:user) }
+  let_it_be(:user) { create(:user, organization: 'GitLab') }
   let(:verdict_value) { nil }
   let(:error_value) { "" }
-  let(:issue) { create(:issue) }
+  let_it_be(:issue) { create(:issue, description: 'Test issue description') }
 
   let(:response) do
     verdict = ::Spamcheck::SpamVerdict.new
@@ -48,4 +48,56 @@ RSpec.describe Gitlab::Spamcheck::Client do
       end
     end
   end
+
+  describe "#build_issue_protobuf", :aggregate_failures do
+    it 'builds the expected protobuf object' do
+      cxt = { action: :create }
+      issue_pb = described_class.new.send(:build_issue_protobuf,
+                                          issue: issue, user: user,
+                                          context: cxt)
+      expect(issue_pb.title).to eq issue.title
+      expect(issue_pb.description).to eq issue.description
+      expect(issue_pb.user_in_project). to be false
+      expect(issue_pb.created_at).to eq timestamp_to_protobuf_timstampe(issue.created_at)
+      expect(issue_pb.updated_at).to eq timestamp_to_protobuf_timstampe(issue.updated_at)
+      expect(issue_pb.action).to be ::Spamcheck::Action.lookup(::Spamcheck::Action::CREATE)
+      expect(issue_pb.user.username).to eq user.username
+    end
+  end
+
+  describe '#build_user_proto_buf', :aggregate_failures do
+    it 'builds the expected protobuf object' do
+      user_pb = described_class.new.send(:build_user_protobuf, user)
+      expect(user_pb.username).to eq user.username
+      expect(user_pb.org).to eq user.organization
+      expect(user_pb.created_at).to eq timestamp_to_protobuf_timstampe(user.created_at)
+      expect(user_pb.emails.count).to be 1
+      expect(user_pb.emails.first.email).to eq user.email
+      expect(user_pb.emails.first.verified).to eq user.confirmed?
+    end
+
+    context 'when user has multiple email addresses' do
+      let(:secondary_email) {create(:email, :confirmed, user: user)}
+
+      before do
+        user.emails << secondary_email
+      end
+
+      it 'adds emsils to the user pb object' do
+        user_pb = described_class.new.send(:build_user_protobuf, user)
+        expect(user_pb.emails.count).to eq 2
+        expect(user_pb.emails.first.email).to eq user.email
+        expect(user_pb.emails.first.verified).to eq user.confirmed?
+        expect(user_pb.emails.last.email).to eq secondary_email.email
+        expect(user_pb.emails.last.verified).to eq secondary_email.confirmed?
+      end
+    end
+  end
+
+  private
+
+  def timestamp_to_protobuf_timstampe(timestamp)
+    Google::Protobuf::Timestamp.new(seconds: timestamp.to_time.to_i,
+                                    nanos: timestamp.to_time.nsec)
+  end
 end

spec/services/spam/spam_action_service_spec.rb

@@ -9,11 +9,11 @@ RSpec.describe Spam::SpamActionService do
   let(:issue) { create(:issue, project: project, author: user) }
   let(:fake_ip) { '1.2.3.4' }
   let(:fake_user_agent) { 'fake-user-agent' }
-  let(:fake_referrer) { 'fake-http-referrer' }
+  let(:fake_referer) { 'fake-http-referer' }
   let(:env) do
     { 'action_dispatch.remote_ip' => fake_ip,
       'HTTP_USER_AGENT' => fake_user_agent,
-      'HTTP_REFERRER' => fake_referrer }
+      'HTTP_REFERER' => fake_referer }
   end
 
   let_it_be(:project) { create(:project, :public) }
@@ -80,7 +80,7 @@ RSpec.describe Spam::SpamActionService do
       {
         ip_address: fake_ip,
         user_agent: fake_user_agent,
-        referrer: fake_referrer
+        referer: fake_referer
       }
     end
 

spec/services/spam/spam_verdict_service_spec.rb

@@ -7,11 +7,11 @@ RSpec.describe Spam::SpamVerdictService do
 
   let(:fake_ip) { '1.2.3.4' }
   let(:fake_user_agent) { 'fake-user-agent' }
-  let(:fake_referrer) { 'fake-http-referrer' }
+  let(:fake_referer) { 'fake-http-referer' }
   let(:env) do
     { 'action_dispatch.remote_ip' => fake_ip,
       'HTTP_USER_AGENT' => fake_user_agent,
-      'HTTP_REFERRER' => fake_referrer }
+      'HTTP_REFERER' => fake_referer }
   end
 
   let(:request) { double(:request, env: env) }
@@ -28,7 +28,7 @@ RSpec.describe Spam::SpamVerdictService do
 
     before do
       allow(service).to receive(:akismet_verdict).and_return(nil)
-      allow(service).to receive(:external_verdict).and_return(nil)
+      allow(service).to receive(:spamcheck_verdict).and_return(nil)
     end
 
     context 'if all services return nil' do
@@ -63,7 +63,7 @@ RSpec.describe Spam::SpamVerdictService do
       context 'and they are supported' do
         before do
           allow(service).to receive(:akismet_verdict).and_return(DISALLOW)
-          allow(service).to receive(:external_verdict).and_return(BLOCK_USER)
+          allow(service).to receive(:spamcheck_verdict).and_return(BLOCK_USER)
         end
 
         it 'renders the more restrictive verdict' do
@@ -74,7 +74,7 @@ RSpec.describe Spam::SpamVerdictService do
       context 'and one is supported' do
         before do
           allow(service).to receive(:akismet_verdict).and_return('nonsense')
-          allow(service).to receive(:external_verdict).and_return(BLOCK_USER)
+          allow(service).to receive(:spamcheck_verdict).and_return(BLOCK_USER)
         end
 
         it 'renders the more restrictive verdict' do
@@ -85,7 +85,7 @@ RSpec.describe Spam::SpamVerdictService do
       context 'and none are supported' do
         before do
           allow(service).to receive(:akismet_verdict).and_return('nonsense')
-          allow(service).to receive(:external_verdict).and_return('rubbish')
+          allow(service).to receive(:spamcheck_verdict).and_return('rubbish')
         end
 
         it 'renders the more restrictive verdict' do
@@ -150,8 +150,8 @@ RSpec.describe Spam::SpamVerdictService do
     end
   end
 
-  describe '#external_verdict' do
-    subject { service.send(:external_verdict) }
+  describe '#spamcheck_verdict' do
+    subject { service.send(:spamcheck_verdict) }
 
     context 'if a Spam Check endpoint enabled and set to a URL' do
       let(:spam_check_body) { {} }
@@ -191,7 +191,7 @@ RSpec.describe Spam::SpamVerdictService do
               allow(Recaptcha).to receive(:enabled?).and_return(true)
             end
 
-            it 'returns ALLOW' do
+            it 'returns CONDITIONAL_ALLOW' do
               expect(subject).to eq CONDITIONAL_ALLOW
             end
           end