Merge pull request #60 from davydovanton/fix/fix-vacancy-query

Fix vacancy query logic

Author: davydovanton

apps/web/controllers/vacancies/index.rb

@@ -36,9 +36,9 @@ def call(params)
         private
 
         def search_query
-          query_attributes = params[:query] ? search_query_parser.call(params[:query]) : EMPTY_SEARCH_QUERY
-          initial_attributes = { remote: nil, position_type: nil, location: nil }
-          search_options_mapper.call(initial_attributes.merge(query_attributes))
+          search_options_mapper.call(
+            search_query_parser.call(params[:query])
+          )
         end
       end
     end

lib/core/libs/search_query_parser.rb

@@ -9,7 +9,11 @@ class SearchQueryParser
 
     SEPARATOR_CHAR = ':'
 
+    EMPTY_RESULT = { text: nil }.freeze
+
     def call(query)
+      return EMPTY_RESULT if query.nil? || query.empty?
+
       scanner = StringScanner.new(query.to_s)
       options = scan_options(scanner)
       text = scanner.scan(/.+/)

lib/core/queries/vacancy.rb

@@ -2,19 +2,21 @@
 
 module Queries
   class Vacancy
+    EMPTY_HASH = {}.freeze
+
     attr_reader :repo
 
     def initialize(repo = VacancyRepository.new)
       @repo = repo
     end
 
-    def all_with_contact(limit:, page:, search_query:)
-      all_with_contact_relation(limit: limit, page: page, search_query: search_query).to_a
+    def all_with_contact(limit:, page:, search_query: nil)
+      all_with_contact_relation(limit: limit, page: page, search_query: search_query || EMPTY_HASH).to_a
     end
 
-    def pager_for_all_with_contact(limit:, page:, search_query:)
+    def pager_for_all_with_contact(limit:, page:, search_query: nil)
       Hanami::Pagination::Pager.new(
-        all_with_contact_relation(limit: limit, page: page, search_query: search_query).pager
+        all_with_contact_relation(limit: limit, page: page, search_query: search_query || EMPTY_HASH).pager
       )
     end
 
@@ -26,23 +28,24 @@ def pager_for_all_with_contact(limit:, page:, search_query:)
       location: ->(query, filter_value) { query.where { location.ilike("%#{filter_value}%") } }
     }.freeze
 
-    def new_all_with_contact_relation(limit:, page:, search_query:)
-      query = repo.aggregate(:contact)
-                  .where(published: true, archived: false, deleted_at: nil)
+    def all_with_contact_relation(limit:, page:, search_query:)
+      query = base_query
+
       search_query.to_h.each do |key, value|
+        next if value.nil?
+
         modifier = QUERY_MODIFIERS[key]
-        query = modifier.call(query, value) if modifier && value
+        query = modifier.call(query, value) if modifier
       end
-      query.map_to(::Vacancy).order { created_at.desc }
-           .per_page(limit).page(page || 1)
+
+      query.per_page(limit).page(page || 1)
     end
-              
-    def all_with_contact_relation(limit:, page:)
+
+    def base_query
       repo.aggregate(:contact)
-          .where(published: true, deleted_at: nil)
+          .where(published: true, archived: false, deleted_at: nil)
           .where('archived_at > ?', Date.today)
           .map_to(::Vacancy).order { created_at.desc }
-          .per_page(limit).page(page || 1)
     end
   end
 end

lib/vacancies/entities/search_options.rb

@@ -3,6 +3,8 @@
 module Vacancies
   module Entities
     class SearchOptions < Dry::Struct
+      constructor_type :schema
+
       attribute :remote, Core::Types::Strict::Bool.optional.default(nil)
       attribute :position_type, Core::Types::VacancyPositionTypes.optional.default(nil)
       attribute :location, Core::Types::Strict::String.optional.default(nil)

lib/vacancies/mappers/search_options.rb

@@ -2,17 +2,25 @@
 
 module Vacancies
   module Mappers
-    Container = Module.new do
-      extend Transproc::Registry
-      import Transproc::HashTransformations
-      import Transproc::Coercions
-      import Transproc::ClassTransformations
-    end
+    class SearchOptions
+      def call(params)
+        payload = {
+          remote: to_bool(params[:remote]),
+          position_type: params[:position_type],
+          location: params[:location]
+        }
+
+        Vacancies::Entities::SearchOptions.new(payload)
+      end
+
+      private
 
-    class SearchOptions < Transproc::Transformer[Container]
-      symbolize_keys
-      map_value :remote, t(:to_boolean)
-      constructor_inject ::Vacancies::Entities::SearchOptions
+      def to_bool(value)
+        case value
+        when 'true'  then true
+        when 'false' then false
+        end
+      end
     end
   end
 end

spec/core/libs/search_query_parser_spec.rb

@@ -3,10 +3,12 @@
 RSpec.describe Libs::SearchQueryParser do
   let(:parser) { described_class.new }
 
+  it { expect(parser.call(nil)).to eq(text: nil) }
   it { expect(parser.call('')).to eq(text: nil) }
   it { expect(parser.call('text')).to eq(text: 'text') }
   it { expect(parser.call('author:davydovanton')).to eq(author: 'davydovanton', text: nil) }
   it { expect(parser.call('tag:test tag:other text')).to eq(tag: %w[test other], text: 'text') }
   it { expect(parser.call('author:davy lang:ruby text')).to eq(author: 'davy', lang: 'ruby', text: 'text') }
   it { expect(parser.call(' author:davy lang:ruby text')).to eq(author: 'davy', lang: 'ruby', text: 'text') }
+  it { expect(parser.call('remote:true location:moscow text')).to eq(text: 'text', remote: 'true', location: 'moscow') }
 end

spec/core/queries/vacancy_spec.rb

@@ -2,7 +2,6 @@
 
 RSpec.describe Queries::Vacancy, type: :query do
   let(:repo) { described_class.new }
-  let(:query_attributes) { return { remote: nil, position_type: nil, location: nil } }
   let(:search_query) { Vacancies::Entities::SearchOptions.new }
 
   describe '#all_with_contact' do
@@ -16,8 +15,8 @@
       Fabricate.create(
         :vacancy,
         published: published,
-        archived: archived,
         deleted_at: deleted_at,
+        archived_at: archived_at,
         remote_available: remote_available,
         position_type: position_type,
         location: location
@@ -34,7 +33,7 @@
       it { expect(subject.first.contact).to be_a(Contact) }
 
       context 'and remote in search_query is true' do
-        let(:search_query) { Vacancies::Entities::SearchOptions.new(query_attributes.merge(remote: true)) }
+        let(:search_query) { Vacancies::Entities::SearchOptions.new(remote: true) }
 
         it { expect(subject).to eq([]) }
 
@@ -45,8 +44,15 @@
         end
       end
 
+      context 'and remote in search_query is false' do
+        let(:search_query) { Vacancies::Entities::SearchOptions.new(remote: false) }
+        let(:remote_available) { true }
+
+        it { expect(subject.count).to eq(0) }
+      end
+
       context 'and position_type in search_query is equal "other"' do
-        let(:search_query) { Vacancies::Entities::SearchOptions.new(query_attributes.merge(position_type: 'other')) }
+        let(:search_query) { Vacancies::Entities::SearchOptions.new(position_type: 'other') }
 
         it { expect(subject).to eq([]) }
 
@@ -58,7 +64,7 @@
       end
 
       context 'and location in search query is not empty' do
-        let(:search_query) { Vacancies::Entities::SearchOptions.new(query_attributes.merge(location: 'VASYUKI')) }
+        let(:search_query) { Vacancies::Entities::SearchOptions.new(location: 'VASYUKI') }
 
         it { expect(subject).to eq([]) }
 

spec/vacancies/mappers/search_options_spec.rb

@@ -3,15 +3,31 @@
 RSpec.describe Vacancies::Mappers::SearchOptions, type: :mapper do
   subject { described_class.new.call(search_options_hash) }
 
-  let(:search_options_hash) { { remote: nil, position_type: 'other', location: 'New Vasyuki' } }
+  let(:search_options_hash) { {} }
 
   it { expect(subject).to be_a(Vacancies::Entities::SearchOptions) }
-  it { expect(subject.position_type).to eq(search_options_hash[:position_type]) }
-  it { expect(subject.location).to eq(search_options_hash[:location]) }
 
   context 'when remote is string equaled "true"' do
-    let(:search_options_hash) { { remote: 'true', position_type: nil, location: nil } }
+    let(:search_options_hash) { { remote: 'true', position_type: 'part_time' } }
 
-    it { expect(subject.remote).to be_truthy }
+    it { expect(subject.remote).to eq true }
+    it { expect(subject.position_type).to eq 'part_time' }
+    it { expect(subject.location).to eq nil }
+  end
+
+  context 'when remote is string equaled "false"' do
+    let(:search_options_hash) { { remote: 'false', position_type: nil, location: 'New Vasyuki' } }
+
+    it { expect(subject.remote).to eq false }
+    it { expect(subject.position_type).to eq nil }
+    it { expect(subject.location).to eq 'New Vasyuki' }
+  end
+
+  context 'when position_type is invalid' do
+    let(:search_options_hash) { { position_type: 'anything' } }
+
+    it do
+      expect { subject }.to raise_error(Dry::Struct::Error)
+    end
   end
 end

spec/web/features/index_page_spec.rb

@@ -9,6 +9,7 @@
     Fabricate(:vacancy)
     Fabricate(:vacancy, position: 'Ruby developer')
     Fabricate(:vacancy, position: 'Java developer', published: false)
+    Fabricate(:vacancy, position: 'C# developer',   remote_available: true)
   end
 
   it 'returns a index page with all vacancies' do
@@ -17,6 +18,38 @@
     expect(page.status_code).to eq 200
     expect(page.body).to include 'Senior mecha pilot'
     expect(page.body).to include 'Ruby developer'
+    expect(page.body).to include 'C# developer'
+
     expect(page.body).to_not include 'Java developer'
   end
+
+  context 'with remote:true query parameter' do
+    let(:url) { '/?query=remote:true' }
+
+    it 'returns a index page with all vacancies' do
+      visit(url)
+
+      expect(page.status_code).to eq 200
+      expect(page.body).to include 'C# developer'
+
+      expect(page.body).to_not include 'Senior mecha pilot'
+      expect(page.body).to_not include 'Ruby developer'
+      expect(page.body).to_not include 'Java developer'
+    end
+  end
+
+  context 'with remote:false query parameter' do
+    let(:url) { '/?query=remote:false' }
+
+    it 'returns a index page with all vacancies' do
+      visit(url)
+
+      expect(page.status_code).to eq 200
+      expect(page.body).to_not include 'C# developer'
+
+      expect(page.body).to include 'Senior mecha pilot'
+      expect(page.body).to include 'Ruby developer'
+      expect(page.body).to_not include 'Java developer'
+    end
+  end
 end