[TBT-381] Feedback incorporated

Author: mshahzaib-travis

lib/travis/api/app/endpoint/assembla.rb

@@ -12,10 +12,9 @@ class Endpoint
     class Assembla < Endpoint
       include Travis::Api::App::JWTUtils
 
-      REQUIRED_JWT_FIELDS = %w[name email login space_id id access_token refresh_token].freeze
+      REQUIRED_JWT_FIELDS = %w[name email login space_id id refresh_token].freeze
       CLUSTER_HEADER = 'HTTP_X_ASSEMBLA_CLUSTER'.freeze
 
-
       set prefix: '/assembla'
       set :check_auth, false
 
@@ -30,13 +29,11 @@ class Assembla < Endpoint
         org = service.find_or_create_organization(user)
         service.create_org_subscription(user, org.id)
 
-
-        response = {
+        {
           user_id: user.id,
           login: user.login,
-          token: user.token,
-          status: 'signed_in'
-        }.to_json
+          token: user.token
+        }
       end
 
       private
@@ -51,7 +48,7 @@ def validate_request!
       def check_required_fields
         missing = REQUIRED_JWT_FIELDS.select { |f| @jwt_payload[f].nil? || @jwt_payload[f].to_s.strip.empty? }
         unless missing.empty?
-          halt 400, { error: 'Missing required fields', missing: missing }.to_json
+          halt 400, { error: 'Missing required fields', missing: missing }
         end
       end
 
@@ -62,7 +59,7 @@ def deep_integration_enabled?
       def valid_asm_cluster?
         allowed = Array(Travis.config.assembla_clusters.to_s.split(','))
         cluster = request.env[CLUSTER_HEADER]
-        !cluster.nil? && allowed.include?(cluster)
+        allowed.include?(cluster)
       end
     end
   end

lib/travis/api/app/jwt_utils.rb

@@ -1,20 +1,19 @@
 class Travis::Api::App
   module JWTUtils
     def extract_jwt_token(request)
-      request.env['HTTP_AUTHORIZATION']&.split(' ')&.last
+      request.env['HTTP_AUTHORIZATION']&.split&.last
     end
 
     def verify_jwt(request)
-      secret = Travis.config.assembla_jwt_secret
       token = extract_jwt_token(request)
 
-      halt 401, { error: "Missing JWT" }.to_json  unless token
+      halt 401, { error: "Missing JWT" } unless token
 
       begin
-        decoded, = JWT.decode(token, secret, true, { algorithm: 'HS256' })
+        decoded, = JWT.decode(token, Travis.config.assembla_jwt_secret, true, algorithm: 'HS256' )
         decoded
       rescue JWT::DecodeError => e
-        halt 401, { error: "Invalid JWT: #{e.message}" }.to_json
+        halt 401, { error: "Invalid JWT: #{e.message}" }
       end
     end
   end

lib/travis/config/defaults.rb

@@ -107,9 +107,10 @@ def fallback_logs_api_auth_token
             antifraud:            { captcha_max_failed_attempts: 3, captcha_block_duration: 24, credit_card_max_failed_attempts: 3, credit_card_block_duration: 24 },
             legacy_roles:         false,
             internal_users:       [{id: 0, login: 'cron'}],
-            deep_integration_enabled: ENV['DEEP_INTEGRATION_ENABLED'],
-            assembla_clusters: ENV['ASSEMBLA_CLUSTERS'],
-            assembla_jwt_secret: ENV['ASSEMBLA_JWT_SECRET']
+            assembla_clusters: 'eu, us',
+            deep_integration_enabled: false,
+            assembla_jwt_secret: 'assembla_jwt_secret',
+            beta_plan_name: 'beta_plan'
 
     default :_access => [:key]
 

lib/travis/services/assembla_user_service.rb

@@ -3,35 +3,39 @@ module Services
     class AssemblaUserService
       class SyncError < StandardError; end
 
+      BILLING_COUNTRY = 'Poland'
+      BILLING_ADDRESS = "System-generated for user %{login} (%{id})"
+      BILLING_CITY = "AutoCity-%{id}"
+      BILLING_ZIP = "000%{id}"
+
       def initialize(payload)
         @payload = payload
       end
 
       def find_or_create_user
-        attrs = {
+        user = ::User.find_or_initialize_by(
+          name: @payload['name'],
           vcs_id: @payload['id'],
           email: @payload['email'],
           login: @payload['login'],
           vcs_type: 'AssemblaUser'
-        }
-
-        user = ::User.find_or_create_by!(attrs)
-        user.update(vcs_oauth_token: @payload['refresh_token'])
+        )
+        user.vcs_oauth_token = @payload['refresh_token']
+        user.save!
         sync_user(user.id)
         user
       end
 
       def find_or_create_organization(user)
-        attrs = {
+        user.organizations.find_or_create_by!(
           vcs_id: @payload['space_id'], 
           vcs_type: 'AssemblaOrganization'
-        }
-        user.organizations.find_or_create_by(attrs)
+        )
       end
 
       def create_org_subscription(user, organization_id)
-        client = Travis::API::V3::BillingClient.new(user.id)
-        client.create_v2_subscription(subscription_params(user, organization_id))
+        billing_client = Travis::API::V3::BillingClient.new(user.id)
+        billing_client.create_v2_subscription(subscription_params(user, organization_id))
       rescue => e
         { error: true, details: e.message }
       end
@@ -46,7 +50,7 @@ def sync_user(user_id)
 
       def subscription_params(user, organization_id)
         {
-          'plan' => 'beta_plan',
+          'plan' => Travis.config.beta_plan_name,
           'organization_id' => organization_id,
           'billing_info' => billing_info(user),
           'credit_card_info' => { 'token' => nil }
@@ -55,12 +59,12 @@ def subscription_params(user, organization_id)
 
       def billing_info(user)
         {
-          'address' => "System-generated for user #{user.login} (#{user.id})",
-          'city' => "AutoCity-#{user.id}",
-          'country' => 'Poland',
+          'address' => BILLING_ADDRESS % { login: user.login, id: user.id },
+          'city' => BILLING_CITY % { id: user.id },
+          'country' => BILLING_COUNTRY,
           'first_name' => user.name&.split&.first,
           'last_name' => user.name&.split&.last,
-          'zip_code' => "000#{user.id}",
+          'zip_code' => BILLING_ZIP % { id: user.id },
           'billing_email' => user.email
         }
       end

spec/lib/services/assembla_user_service_spec.rb

@@ -1,9 +1,11 @@
 require 'spec_helper'
+require 'factory_bot'
 
 RSpec.describe Travis::Services::AssemblaUserService do
   let(:payload) do
     {
       'id' => '12345',
+      'name' => 'Test User',
       'email' => '[email protected]',
       'login' => 'testuser',
       'refresh_token' => 'refresh123',
@@ -12,39 +14,30 @@
   end
 
   let(:service) { described_class.new(payload) }
-  let(:user) { double('User', id: 1, login: 'testuser', email: '[email protected]', name: 'Test User') }
-  let(:organization) { double('Organization', id: 2) }
-
-  describe '#initialize' do
-    it 'stores the payload' do
-      expect(service.instance_variable_get(:@payload)).to eq(payload)
-    end
-  end
+  let(:user) { FactoryBot.create(:user, vcs_id: payload['id'], email: payload['email'], login: payload['login'], name: payload['name']) }
+  let(:organization) { FactoryBot.create(:org, vcs_id: payload['space_id'], vcs_type: 'AssemblaOrganization') }
 
   describe '#find_or_create_user' do
     let(:expected_attrs) do
       {
-        vcs_id: '12345',
-        email: '[email protected]',
-        login: 'testuser',
+        vcs_id: payload['id'],
+        email: payload['email'],
+        name: payload['name'],
+        login: payload['login'],
         vcs_type: 'AssemblaUser'
       }
     end
 
     before do
-      allow(::User).to receive(:find_or_create_by!).with(expected_attrs).and_return(user)
-      allow(user).to receive(:update)
       allow(Travis::RemoteVCS::User).to receive(:new).and_return(double(sync: true))
     end
 
     it 'finds or creates a user with correct attributes' do
-      expect(::User).to receive(:find_or_create_by!).with(expected_attrs)
-      service.find_or_create_user
-    end
-
-    it 'returns the user' do
-      result = service.find_or_create_user
-      expect(result).to eq(user)
+      service_user = service.find_or_create_user
+      expect(service_user.login).to eq(expected_attrs[:login])
+      expect(service_user.email).to eq(expected_attrs[:email])
+      expect(service_user.name).to eq(expected_attrs[:name])
+      expect(service_user.vcs_id).to eq(expected_attrs[:vcs_id])
     end
 
     context 'when sync fails' do
@@ -60,60 +53,26 @@
   end
 
   describe '#find_or_create_organization' do
-    let(:organizations_relation) { double('organizations') }
     let(:expected_attrs) do
       {
-        vcs_id: '67890',
+        vcs_id: payload['space_id'],
         vcs_type: 'AssemblaOrganization'
       }
     end
 
-    before do
-      allow(user).to receive(:organizations).and_return(organizations_relation)
-    end
-
     it 'finds or creates organization with correct attributes' do
-      expect(organizations_relation).to receive(:find_or_create_by).with(expected_attrs).and_return(organization)
+      service_org = service.find_or_create_organization(user)
 
-      result = service.find_or_create_organization(user)
-      expect(result).to eq(organization)
+      expect(service_org.vcs_type).to eq(expected_attrs[:vcs_type])
+      expect(service_org.vcs_id).to eq(expected_attrs[:vcs_id])
     end
   end
 
   describe '#create_org_subscription' do
     let(:billing_client) { double('BillingClient') }
-    let(:expected_subscription_params) do
-      {
-        'plan' => 'beta_plan',
-        'organization_id' => 2,
-        'billing_info' => {
-          'address' => 'System-generated for user testuser (1)',
-          'city' => 'AutoCity-1',
-          'country' => 'Poland',
-          'first_name' => 'Test',
-          'last_name' => 'User',
-          'zip_code' => '0001',
-          'billing_email' => '[email protected]'
-        },
-        'credit_card_info' => { 'token' => nil }
-      }
-    end
 
     before do
-      allow(Travis::API::V3::BillingClient).to receive(:new).with(1).and_return(billing_client)
-    end
-
-    it 'creates a billing client with user id' do
-      expect(Travis::API::V3::BillingClient).to receive(:new).with(1)
-      allow(billing_client).to receive(:create_v2_subscription)
-      
-      service.create_org_subscription(user, 2)
-    end
-
-    it 'calls create_v2_subscription with correct params' do
-      expect(billing_client).to receive(:create_v2_subscription).with(expected_subscription_params)
-      
-      service.create_org_subscription(user, 2)
+      allow(Travis::API::V3::BillingClient).to receive(:new).with(user.id).and_return(billing_client)
     end
 
     context 'when billing client raises an error' do
@@ -122,23 +81,10 @@
       it 'returns error hash' do
         allow(billing_client).to receive(:create_v2_subscription).and_raise(error)
 
-        result = service.create_org_subscription(user, 2)
-        expect(result).to eq({ error: true, details: 'Billing error' })
-      end
-    end
-
-    context 'when user has no name' do
-      let(:user_without_name) { double('User', id: 1, login: 'testuser', email: '[email protected]', name: nil) }
-
-      it 'handles nil name gracefully' do
-        expected_params = expected_subscription_params.dup
-        expected_params['billing_info']['first_name'] = nil
-        expected_params['billing_info']['last_name'] = nil
-
-        expect(billing_client).to receive(:create_v2_subscription).with(expected_params)
-        
-        service.create_org_subscription(user_without_name, 2)
+        result = service.create_org_subscription(user, organization.id)
+        expect(result[:error]).to be_truthy
+        expect(result[:details]).to be_present
       end
     end
   end
-end
+end

spec/unit/endpoint/assembla_spec.rb

@@ -22,13 +22,18 @@
   let(:organization) { double('Organization', id: 1) }
   let(:organizations) { double('Organizations') }
   let(:subscription_response) { { 'status' => 'subscribed' } }
+  let(:assembla_cluster) { 'cluster1' }
 
   before do
     Travis.config[:deep_integration_enabled] = true
-    Travis.config[:assembla_clusters] = 'cluster1'
+    Travis.config[:assembla_clusters] = assembla_cluster
     Travis.config[:assembla_jwt_secret] = jwt_secret
 
-    header 'X_ASSEMBLA_CLUSTER', 'cluster1'
+    header 'X_ASSEMBLA_CLUSTER', assembla_cluster
+  end
+
+  after do
+    Travis.config[:deep_integration_enabled] = false
   end
 
   describe 'POST /assembla/login' do
@@ -50,9 +55,8 @@
 
         expect(last_response.status).to eq(200)
         body = JSON.parse(last_response.body)
-        expect(body['login']).to eq('testuser')
+        expect(body['login']).to eq(user.login)
         expect(body['token']).to eq('abc123')
-        expect(body['status']).to eq('signed_in')
       end
     end