From 107fd5e995e51d9c6b3be3a10a0b36d5869d3838 Mon Sep 17 00:00:00 2001 From: Ryan Lue Date: Sun, 10 Feb 2019 13:24:22 +0100 Subject: [PATCH] Refactoring: Migrate ticket_article_store_empty to RSpec --- spec/factories/activity_stream.rb | 12 + spec/factories/history.rb | 11 +- spec/factories/karma/activity_log.rb | 14 + spec/factories/link.rb | 13 +- spec/factories/online_notification.rb | 22 +- spec/factories/recent_view.rb | 17 +- spec/factories/tag.rb | 14 +- spec/factories/tag/object.rb | 5 + spec/factories/ticket/article.rb | 26 +- spec/factories/type_lookup.rb | 17 + spec/models/ticket_spec.rb | 739 ++++++++++++------------ test/unit/ticket_article_store_empty.rb | 74 --- 12 files changed, 466 insertions(+), 498 deletions(-) create mode 100644 spec/factories/activity_stream.rb create mode 100644 spec/factories/karma/activity_log.rb create mode 100644 spec/factories/tag/object.rb create mode 100644 spec/factories/type_lookup.rb delete mode 100644 test/unit/ticket_article_store_empty.rb diff --git a/spec/factories/activity_stream.rb b/spec/factories/activity_stream.rb new file mode 100644 index 000000000..e51da74bc --- /dev/null +++ b/spec/factories/activity_stream.rb @@ -0,0 +1,12 @@ +FactoryBot.define do + factory :activity_stream do + transient do + o { Ticket.first } + end + + association :type, factory: :type_lookup + activity_stream_object_id { ObjectLookup.by_name(o.class.name) } + o_id { o.id } + created_by_id { 1 } + end +end diff --git a/spec/factories/history.rb b/spec/factories/history.rb index 0e91be58b..894d20f64 100644 --- a/spec/factories/history.rb +++ b/spec/factories/history.rb @@ -1,8 +1,15 @@ FactoryBot.define do factory :history do + transient do + o { Ticket.first } + end + association :history_type, factory: :'history/type' - association :history_object, factory: :'history/object' - o_id { history_object.name.constantize.pluck(:id).sample } + o_id { o.id } created_by_id { 1 } + + history_object_id do + History::Object.lookup(name: o.class.name)&.id || create(:'history/object', name: o.class.name).id + end end end diff --git a/spec/factories/karma/activity_log.rb b/spec/factories/karma/activity_log.rb new file mode 100644 index 000000000..7a959871a --- /dev/null +++ b/spec/factories/karma/activity_log.rb @@ -0,0 +1,14 @@ +FactoryBot.define do + factory :'karma/activity_log', aliases: %i[karma_activity_log] do + transient do + o { Ticket.first } + end + + o_id { o.id } + object_lookup_id { ObjectLookup.by_name(o.class.name) } + user_id { 1 } + activity_id { Karma::Activity.pluck(:id).sample } + score { 100 } + score_total { 100 } + end +end diff --git a/spec/factories/link.rb b/spec/factories/link.rb index 5365c4ade..bfa888cc7 100644 --- a/spec/factories/link.rb +++ b/spec/factories/link.rb @@ -1,7 +1,14 @@ FactoryBot.define do factory :link do - link_type_id { Link::Type.find_by(name: 'normal').id } - link_object_source_id { Link::Object.find_by(name: 'Ticket').id } - link_object_target_id { Link::Object.find_by(name: 'Ticket').id } + transient do + from { Ticket.first } + to { Ticket.last } + end + + link_type_id { Link::Type.find_by(name: 'normal').id } + link_object_source_id { Link::Object.find_by(name: 'Ticket').id } + link_object_target_id { Link::Object.find_by(name: 'Ticket').id } + link_object_source_value { from.id } + link_object_target_value { to.id } end end diff --git a/spec/factories/online_notification.rb b/spec/factories/online_notification.rb index 7b7bf169f..ee644ab85 100644 --- a/spec/factories/online_notification.rb +++ b/spec/factories/online_notification.rb @@ -1,13 +1,17 @@ FactoryBot.define do factory :online_notification do - object_lookup_id { ObjectLookup.by_name('Ticket') } - o_id 1 - type_lookup_id { TypeLookup.by_name('updated') } - seen false - user_id 1 - created_by_id 1 - updated_by_id 1 - created_at Time.zone.now - updated_at Time.zone.now + transient do + o { Ticket.first } + end + + object_lookup_id { ObjectLookup.by_name(o.class.name) } + o_id { o.id } + type_lookup_id { TypeLookup.by_name('updated') } + seen { false } + user_id { 1 } + created_by_id { 1 } + updated_by_id { 1 } + created_at { Time.zone.now } + updated_at { Time.zone.now } end end diff --git a/spec/factories/recent_view.rb b/spec/factories/recent_view.rb index 29889a35c..b6f9adc94 100644 --- a/spec/factories/recent_view.rb +++ b/spec/factories/recent_view.rb @@ -1,23 +1,12 @@ FactoryBot.define do factory :recent_view do transient do - type { :ticket } + o { Ticket.first } user_role { :agent } end - recent_view_object_id { ObjectLookup.by_name(type.to_s.camelcase) } - - # select a random record of the given object class - o_id do - random_function = case ActiveRecord::Base.connection_config[:adapter] - when 'mysql2' - 'RAND' - when 'postgresql' - 'RANDOM' - end - - type.to_s.camelcase.constantize.order("#{random_function}()").first.id - end + recent_view_object_id { ObjectLookup.by_name(o.class.name) } + o_id { o.id } # assign to an existing user, if possible created_by_id do diff --git a/spec/factories/tag.rb b/spec/factories/tag.rb index 3794e6f2b..fbd28e0bc 100644 --- a/spec/factories/tag.rb +++ b/spec/factories/tag.rb @@ -1,7 +1,15 @@ FactoryBot.define do factory :tag do - tag_object_id { Tag::Object.lookup_by_name_and_create('Ticket').id } - tag_item_id { Tag::Item.lookup_by_name_and_create('blub').id } - created_by_id 1 + transient do + o { Ticket.first } + end + + tag_item_id { Tag::Item.lookup_by_name_and_create('blub').id } + o_id { o.id } + created_by_id { 1 } + + tag_object_id do + Tag::Object.lookup(name: o.class.name)&.id || create(:'tag/object', name: o.class.name).id + end end end diff --git a/spec/factories/tag/object.rb b/spec/factories/tag/object.rb new file mode 100644 index 000000000..8f852404c --- /dev/null +++ b/spec/factories/tag/object.rb @@ -0,0 +1,5 @@ +FactoryBot.define do + factory :'tag/object', aliases: %i[tag_object] do + name { (ApplicationModel.descendants.select(&:any?).map(&:name) - Tag::Object.pluck(:name)).sample } + end +end diff --git a/spec/factories/ticket/article.rb b/spec/factories/ticket/article.rb index 35b4fc683..8d279614e 100644 --- a/spec/factories/ticket/article.rb +++ b/spec/factories/ticket/article.rb @@ -1,25 +1,25 @@ FactoryBot.define do factory :'ticket/article', aliases: %i[ticket_article] do transient do - type_name 'email' - sender_name 'Customer' + type_name { 'email' } + sender_name { 'Customer' } end ticket - from 'factory-customer-1@example.com' - to 'factory-customer-1@example.com' - subject 'factory article' - message_id 'factory@id_com_1' - body 'some message 123' - internal false - sender { Ticket::Article::Sender.find_by(name: sender_name) } - type { Ticket::Article::Type.find_by(name: type_name) } - updated_by_id 1 - created_by_id 1 + from { 'factory-customer-1@example.com' } + to { 'factory-customer-1@example.com' } + subject { 'factory article' } + message_id { 'factory@id_com_1' } + body { 'some message 123' } + internal { false } + sender { Ticket::Article::Sender.find_by(name: sender_name) } + type { Ticket::Article::Type.find_by(name: type_name) } + updated_by_id { 1 } + created_by_id { 1 } factory :twitter_article do transient do - type_name 'twitter status' + type_name { 'twitter status' } end association :ticket, factory: :twitter_ticket diff --git a/spec/factories/type_lookup.rb b/spec/factories/type_lookup.rb new file mode 100644 index 000000000..eccd1d79b --- /dev/null +++ b/spec/factories/type_lookup.rb @@ -0,0 +1,17 @@ +FactoryBot.define do + factory :type_lookup do + name do + # The following line ensures that the name generated by Faker + # does not conflict with any existing names in the DB. + # There's a special syntax for this + # (Faker::Verb.unique.exclude(:past_participle, [], Ticket::StateType.pluck(:name)), + # but it's not available yet in the current release of Faker (1.9.1). + Faker::Verb.unique + .instance_variable_get(:@previous_results) + .dig([:base, []]) + .merge(TypeLookup.pluck(:name)) + + Faker::Verb.unique.base + end + end +end diff --git a/spec/models/ticket_spec.rb b/spec/models/ticket_spec.rb index 2d9fd13df..2f71a7cd1 100644 --- a/spec/models/ticket_spec.rb +++ b/spec/models/ticket_spec.rb @@ -8,394 +8,373 @@ RSpec.describe Ticket, type: :model do it_behaves_like 'CanBeImported' it_behaves_like 'CanLookup' - describe '#merge_to' do - - it 'reassigns all links to the target ticket after merge' do - source_ticket = create(:ticket) - target_ticket = create(:ticket) - - important_ticket1 = create(:ticket) - important_ticket2 = create(:ticket) - important_ticket3 = create(:ticket) - - create(:link, link_object_source_value: source_ticket.id, link_object_target_value: important_ticket1.id) - create(:link, link_object_source_value: source_ticket.id, link_object_target_value: important_ticket2.id) - create(:link, link_object_source_value: source_ticket.id, link_object_target_value: important_ticket3.id) - - source_ticket.merge_to( - ticket_id: target_ticket.id, - user_id: 1, - ) - - links = Link.list( - link_object: 'Ticket', - link_object_value: target_ticket.id, - ) - - expected_ticket_ids = [source_ticket.id, important_ticket1.id, important_ticket2.id, important_ticket3.id ] - check_ticket_ids = links.collect { |link| link['link_object_value'] } - - expect(check_ticket_ids).to match_array(expected_ticket_ids) - end - - it 'prevents cross merging tickets' do - source_ticket = create(:ticket) - target_ticket = create(:ticket) - - result = source_ticket.merge_to( - ticket_id: target_ticket.id, - user_id: 1, - ) - expect(result).to be(true) - - expect do - result = target_ticket.merge_to( - ticket_id: source_ticket.id, - user_id: 1, - ) - end.to raise_error('ticket already merged, no merge into merged ticket possible') - end - - it 'prevents merging ticket in it self' do - source_ticket = create(:ticket) - - expect do - result = source_ticket.merge_to( - ticket_id: source_ticket.id, - user_id: 1, - ) - end.to raise_error('Can\'t merge ticket with it self!') - end - - end - - describe '.create' do - it 'handles NULL byte in title' do - expect(create(:ticket, title: "some title \u0000 123")) - .to be_persisted - end - end - - describe '#destroy' do - - it 'deletes all related objects before destroy' do - ApplicationHandleInfo.current = 'application_server' - - source_ticket = create(:ticket) - - # create some links - important_ticket1 = create(:ticket) - important_ticket2 = create(:ticket) - important_ticket3 = create(:ticket) - - # create some articles - create(:ticket_article, ticket_id: source_ticket.id) - create(:ticket_article, ticket_id: source_ticket.id) - create(:ticket_article, ticket_id: source_ticket.id) - - create(:link, link_object_source_value: source_ticket.id, link_object_target_value: important_ticket1.id) - create(:link, link_object_source_value: important_ticket2.id, link_object_target_value: source_ticket.id) - create(:link, link_object_source_value: source_ticket.id, link_object_target_value: important_ticket3.id) - - create(:online_notification, o_id: source_ticket.id) - create(:tag, o_id: source_ticket.id) - - Observer::Transaction.commit - Scheduler.worker(true) - - # get before destroy - activities = ActivityStream.where( - activity_stream_object_id: ObjectLookup.by_name('Ticket'), - o_id: source_ticket.id, - ) - links = Link.list( - link_object: 'Ticket', - link_object_value: source_ticket.id - ) - articles = Ticket::Article.where(ticket_id: source_ticket.id) - history = History.list('Ticket', source_ticket.id, nil, true) - karma_log = Karma::ActivityLog.where( - object_lookup_id: ObjectLookup.by_name('Ticket'), - o_id: source_ticket.id, - ) - online_notifications = OnlineNotification.where( - object_lookup_id: ObjectLookup.by_name('Ticket'), - o_id: source_ticket.id, - ) - recent_views = OnlineNotification.where( - object_lookup_id: ObjectLookup.by_name('Ticket'), - o_id: source_ticket.id, - ) - tags = Tag.tag_list( - object: 'Ticket', - o_id: source_ticket.id, - ) - - # check before destroy - expect(activities.count).to be >= 0 - expect(links.count).to be >= 0 - expect(articles.count).to be >= 0 - expect(history[:list].count).to be >= 0 - expect(karma_log.count).to be >= 0 - expect(online_notifications.count).to be >= 0 - expect(recent_views.count).to be >= 0 - expect(tags.count).to be >= 0 - - # destroy ticket - source_ticket.destroy - - # get after destroy - activities = ActivityStream.where( - activity_stream_object_id: ObjectLookup.by_name('Ticket'), - o_id: source_ticket.id, - ) - links = Link.list( - link_object: 'Ticket', - link_object_value: source_ticket.id - ) - articles = Ticket::Article.where(ticket_id: source_ticket.id) - history = History.list('Ticket', source_ticket.id, nil, true) - karma_log = Karma::ActivityLog.where( - object_lookup_id: ObjectLookup.by_name('Ticket'), - o_id: source_ticket.id, - ) - online_notifications = OnlineNotification.where( - object_lookup_id: ObjectLookup.by_name('Ticket'), - o_id: source_ticket.id, - ) - recent_views = OnlineNotification.where( - object_lookup_id: ObjectLookup.by_name('Ticket'), - o_id: source_ticket.id, - ) - tags = Tag.tag_list( - object: 'Ticket', - o_id: source_ticket.id, - ) - - # check after destroy - expect(activities.count).to be == 0 - expect(links.count).to be == 0 - expect(articles.count).to be == 0 - expect(history[:list].count).to be == 0 - expect(karma_log.count).to be == 0 - expect(online_notifications.count).to be == 0 - expect(recent_views.count).to be == 0 - expect(tags.count).to be == 0 - - end - - end - - describe '#perform_changes' do - - it 'performs a ticket state change on a ticket' do - source_ticket = create(:ticket) - - changes = { - 'ticket.state_id' => { 'value' => Ticket::State.lookup(name: 'closed').id.to_s }, - } - - source_ticket.perform_changes(changes, 'trigger', source_ticket, User.find(1)) - source_ticket.reload - - expect(source_ticket.state.name).to eq('closed') - end - - it 'performs a ticket deletion on a ticket' do - source_ticket = create(:ticket) - - changes = { - 'ticket.state_id' => { 'value' => Ticket::State.lookup(name: 'closed').id.to_s }, - 'ticket.action' => { 'value' => 'delete' }, - } - - source_ticket.perform_changes(changes, 'trigger', source_ticket, User.find(1)) - ticket_with_source_ids = Ticket.where(id: source_ticket.id) - expect(ticket_with_source_ids).to match_array([]) - end - - # Regression test for https://github.com/zammad/zammad/issues/2001 - it 'does not modify its arguments' do - trigger = Trigger.new( - perform: { - 'notification.email' => { - body: "Hello \#{ticket.customer.firstname} \#{ticket.customer.lastname},", - recipient: %w[article_last_sender ticket_owner ticket_customer ticket_agents], - subject: "Autoclose (\#{ticket.title})" - } - } - ) - - expect { Ticket.first.perform_changes(trigger.perform, 'trigger', {}, 1) } - .to not_change { trigger.perform['notification.email'][:body] } - .and not_change { trigger.perform['notification.email'][:subject] } - end - - # Regression test for https://github.com/zammad/zammad/issues/1543 - # - # If a new article fires an email notification trigger, - # and then another article is added to the same ticket - # before that trigger is performed, - # the email template's 'article' var should refer to the originating article, - # not the newest one. - # - # (This occurs whenever one action fires multiple email notification triggers.) - it 'passes the correct article to NotificationFactory::Mailer' do - # required by Ticket#perform_changes for email notifications - Group.first.update(email_address: create(:email_address)) - - ticket = Ticket.first - orig_article = Ticket::Article.where(ticket_id: ticket.id).first - newer_article = create(:ticket_article, ticket_id: ticket.id) - trigger = Trigger.new( - perform: { - 'notification.email' => { - body: '', - recipient: 'ticket_customer', - subject: '' - } - } - ) - - allow(NotificationFactory::Mailer).to receive(:template).and_return('') - - ticket.perform_changes(trigger.perform, 'trigger', { article_id: orig_article.id }, 1) - - expect(NotificationFactory::Mailer) - .to have_received(:template) - .with(hash_including(objects: { ticket: ticket, article: orig_article })) - .at_least(:once) - - expect(NotificationFactory::Mailer) - .not_to have_received(:template) - .with(hash_including(objects: { ticket: ticket, article: newer_article })) - end - end - - describe '#selectors' do - - # https://github.com/zammad/zammad/issues/1769 - it 'does not return multiple results for a single ticket' do - source_ticket = create(:ticket) - source_ticket2 = create(:ticket) - - # create some articles - create(:ticket_article, ticket_id: source_ticket.id, from: 'asdf1@blubselector.de') - create(:ticket_article, ticket_id: source_ticket.id, from: 'asdf2@blubselector.de') - create(:ticket_article, ticket_id: source_ticket.id, from: 'asdf3@blubselector.de') - create(:ticket_article, ticket_id: source_ticket2.id, from: 'asdf4@blubselector.de') - create(:ticket_article, ticket_id: source_ticket2.id, from: 'asdf5@blubselector.de') - create(:ticket_article, ticket_id: source_ticket2.id, from: 'asdf6@blubselector.de') - - condition = { - 'article.from' => { - operator: 'contains', - value: 'blubselector.de', - }, - } - - ticket_count, tickets = Ticket.selectors(condition, 100, nil, 'full') - - expect(ticket_count).to be == 2 - expect(tickets.count).to be == 2 - end - end - - context 'callbacks' do - - describe '#reset_pending_time' do - - it 'resets the pending time on state change' do - ticket = create(:ticket, - state: Ticket::State.lookup(name: 'pending reminder'), - pending_time: Time.zone.now + 2.days) - expect(ticket.pending_time).not_to be nil - - ticket.update!(state: Ticket::State.lookup(name: 'open')) - expect(ticket.pending_time).to be nil - end - - it 'lets handle ActiveRecord nil as new value' do - ticket = create(:ticket) - expect do - ticket.update!(state: nil) - end.to raise_error(ActiveRecord::StatementInvalid) - end - - end - end - - describe '#access?' do - - context 'agent' do - - it 'allows owner access' do - - owner = create(:agent_user) - ticket = create(:ticket, owner: owner) - - expect( ticket.access?(owner, 'full') ).to be(true) - end - - it 'allows group access' do - - agent = create(:agent_user) - group = create(:group) - ticket = create(:ticket, group: group) - - agent.group_names_access_map = { - group.name => 'full', - } - - expect( ticket.access?(agent, 'full') ).to be(true) - end - - it 'prevents unauthorized access' do - agent = create(:agent_user) - ticket = create(:ticket) - - expect( ticket.access?(agent, 'read') ).to be(false) - end - end - - context 'customer' do - - it 'allows assigned access' do - - customer = create(:customer_user) - ticket = create(:ticket, customer: customer) - - expect( ticket.access?(customer, 'full') ).to be(true) - end - - context 'organization' do - - it 'allows access for shared' do - - organization = create(:organization) - assigned = create(:customer_user, organization: organization) - collegue = create(:customer_user, organization: organization) - ticket = create(:ticket, customer: assigned) - - expect( ticket.access?(collegue, 'full') ).to be(true) + subject(:ticket) { create(:ticket) } + + describe 'Class methods:' do + describe '.selectors' do + # https://github.com/zammad/zammad/issues/1769 + context 'when matching multiple tickets, each with multiple articles' do + let(:tickets) { create_list(:ticket, 2) } + + before do + create(:ticket_article, ticket: tickets.first, from: 'asdf1@blubselector.de') + create(:ticket_article, ticket: tickets.first, from: 'asdf2@blubselector.de') + create(:ticket_article, ticket: tickets.first, from: 'asdf3@blubselector.de') + create(:ticket_article, ticket: tickets.last, from: 'asdf4@blubselector.de') + create(:ticket_article, ticket: tickets.last, from: 'asdf5@blubselector.de') + create(:ticket_article, ticket: tickets.last, from: 'asdf6@blubselector.de') end - it 'prevents unshared access' do + let(:condition) do + { + 'article.from' => { + operator: 'contains', + value: 'blubselector.de', + }, + } + end - organization = create(:organization, shared: false) - assigned = create(:customer_user, organization: organization) - collegue = create(:customer_user, organization: organization) - ticket = create(:ticket, customer: assigned) + it 'returns a list of unique tickets (i.e., no duplicates)' do + expect(Ticket.selectors(condition, 100, nil, 'full')) + .to match_array([2, tickets]) + end + end + end + end - expect( ticket.access?(collegue, 'full') ).to be(false) + describe 'Instance methods:' do + describe '#merge_to' do + let(:target_ticket) { create(:ticket) } + + context 'when source ticket has Links' do + let(:linked_tickets) { create_list(:ticket, 3) } + let(:links) { linked_tickets.map { |l| create(:link, from: ticket, to: l) } } + + it 'reassigns all links to the target ticket after merge' do + expect { ticket.merge_to(ticket_id: target_ticket.id, user_id: 1) } + .to change { links.each(&:reload).map(&:link_object_source_value) } + .to(Array.new(3) { target_ticket.id }) end end - it 'prevents unauthorized access' do - customer = create(:customer_user) - ticket = create(:ticket) + context 'when attempting to cross-merge (i.e., to merge B → A after merging A → B)' do + before { target_ticket.merge_to(ticket_id: ticket.id, user_id: 1) } - expect( ticket.access?(customer, 'read') ).to be(false) + it 'raises an error' do + expect { ticket.merge_to(ticket_id: target_ticket.id, user_id: 1) } + .to raise_error('ticket already merged, no merge into merged ticket possible') + end + end + + context 'when attempting to self-merge (i.e., to merge A → A)' do + it 'raises an error' do + expect { ticket.merge_to(ticket_id: ticket.id, user_id: 1) } + .to raise_error("Can't merge ticket with it self!") + end + end + end + + describe '#perform_changes' do + # Regression test for https://github.com/zammad/zammad/issues/2001 + describe 'argument handling' do + let(:perform) do + { + 'notification.email' => { + body: "Hello \#{ticket.customer.firstname} \#{ticket.customer.lastname},", + recipient: %w[article_last_sender ticket_owner ticket_customer ticket_agents], + subject: "Autoclose (\#{ticket.title})" + } + } + end + + it 'does not mutate contents of "perform" hash' do + expect { ticket.perform_changes(perform, 'trigger', {}, 1) } + .not_to change { perform } + end + end + + context 'with "ticket.state_id" key in "perform" hash' do + let(:perform) do + { + 'ticket.state_id' => { + 'value' => Ticket::State.lookup(name: 'closed').id + } + } + end + + it 'changes #state to specified value' do + expect { ticket.perform_changes(perform, 'trigger', ticket, User.first) } + .to change { ticket.reload.state.name }.to('closed') + end + end + + context 'with "ticket.action" => { "value" => "delete" } in "perform" hash' do + let(:perform) do + { + 'ticket.state_id' => { 'value' => Ticket::State.lookup(name: 'closed').id.to_s }, + 'ticket.action' => { 'value' => 'delete' }, + } + end + + it 'performs a ticket deletion on a ticket' do + expect { ticket.perform_changes(perform, 'trigger', ticket, User.first) } + .to change { ticket.destroyed? }.to(true) + end + end + + context 'with a "notification.email" trigger' do + # Regression test for https://github.com/zammad/zammad/issues/1543 + # + # If a new article fires an email notification trigger, + # and then another article is added to the same ticket + # before that trigger is performed, + # the email template's 'article' var should refer to the originating article, + # not the newest one. + # + # (This occurs whenever one action fires multiple email notification triggers.) + context 'when two articles are created before the trigger fires once (race condition)' do + let!(:article) { create(:ticket_article, ticket: ticket) } + let!(:new_article) { create(:ticket_article, ticket: ticket) } + + let(:trigger) do + build(:trigger, + perform: { + 'notification.email' => { + body: '', + recipient: 'ticket_customer', + subject: '' + } + }) + end + + # required by Ticket#perform_changes for email notifications + before { article.ticket.group.update(email_address: create(:email_address)) } + + it 'passes the first article to NotificationFactory::Mailer' do + expect(NotificationFactory::Mailer) + .to receive(:template) + .with(hash_including(objects: { ticket: ticket, article: article })) + .at_least(:once) + .and_call_original + + expect(NotificationFactory::Mailer) + .not_to receive(:template) + .with(hash_including(objects: { ticket: ticket, article: new_article })) + + ticket.perform_changes(trigger.perform, 'trigger', { article_id: article.id }, 1) + end + end + end + end + + describe '#access?' do + context 'when given ticket’s owner' do + it 'returns true for both "read" and "full" privileges' do + expect(ticket.access?(ticket.owner, 'read')).to be(true) + expect(ticket.access?(ticket.owner, 'full')).to be(true) + end + end + + context 'when given the ticket’s customer' do + it 'returns true for both "read" and "full" privileges' do + expect(ticket.access?(ticket.customer, 'read')).to be(true) + expect(ticket.access?(ticket.customer, 'full')).to be(true) + end + end + + context 'when given a user that is neither owner nor customer' do + let(:user) { create(:agent_user) } + + it 'returns false for both "read" and "full" privileges' do + expect(ticket.access?(user, 'read')).to be(false) + expect(ticket.access?(user, 'full')).to be(false) + end + + context 'but the user is an agent with full access to ticket’s group' do + before { user.group_names_access_map = { ticket.group.name => 'full' } } + + it 'returns true for both "read" and "full" privileges' do + expect(ticket.access?(user, 'read')).to be(true) + expect(ticket.access?(user, 'full')).to be(true) + end + end + + context 'but the user is a customer from the same organization as ticket’s customer' do + subject(:ticket) { create(:ticket, customer: customer) } + let(:customer) { create(:customer_user, organization: create(:organization)) } + let(:colleague) { create(:customer_user, organization: customer.organization) } + + context 'and organization.shared is true (default)' do + it 'returns true for both "read" and "full" privileges' do + expect(ticket.access?(colleague, 'read')).to be(true) + expect(ticket.access?(colleague, 'full')).to be(true) + end + end + + context 'but organization.shared is false' do + before { customer.organization.update(shared: false) } + + it 'returns false for both "read" and "full" privileges' do + expect(ticket.access?(colleague, 'read')).to be(false) + expect(ticket.access?(colleague, 'full')).to be(false) + end + end + end + end + end + end + + describe 'Attributes:' do + describe '#pending_time' do + subject(:ticket) { create(:ticket, pending_time: Time.zone.now + 2.days) } + + context 'when #state is updated to any non-"pending" value' do + it 'is reset to nil' do + expect { ticket.update!(state: Ticket::State.lookup(name: 'open')) } + .to change { ticket.pending_time }.to(nil) + end + end + + # Regression test for commit 92f227786f298bad1ccaf92d4478a7062ea6a49f + context 'when #state is updated to nil (violating DB NOT NULL constraint)' do + it 'does not prematurely raise within the callback (#reset_pending_time)' do + expect { ticket.update!(state: nil) } + .to raise_error(ActiveRecord::StatementInvalid) + end + end + end + end + + describe 'Callbacks & Observers -' do + describe 'NULL byte handling (via ChecksAttributeValuesAndLength concern):' do + it 'removes them from title on creation, if necessary (postgres doesn’t like them)' do + expect { create(:ticket, title: "some title \u0000 123") } + .not_to raise_error + end + end + + describe 'Association & attachment management:' do + it 'deletes all related ActivityStreams on destroy' do + create_list(:activity_stream, 3, o: ticket) + + expect { ticket.destroy } + .to change { ActivityStream.exists?(activity_stream_object_id: ObjectLookup.by_name('Ticket'), o_id: ticket.id) } + .to(false) + end + + it 'deletes all related Links on destroy' do + create(:link, from: ticket, to: create(:ticket)) + create(:link, from: create(:ticket), to: ticket) + create(:link, from: ticket, to: create(:ticket)) + + expect { ticket.destroy } + .to change { Link.where('link_object_source_value = :id OR link_object_target_value = :id', id: ticket.id).any? } + .to(false) + end + + it 'deletes all related Articles on destroy' do + create_list(:ticket_article, 3, ticket: ticket) + + expect { ticket.destroy } + .to change { Ticket::Article.exists?(ticket: ticket) } + .to(false) + end + + it 'deletes all related OnlineNotifications on destroy' do + create_list(:online_notification, 3, o: ticket) + + expect { ticket.destroy } + .to change { OnlineNotification.where(object_lookup_id: ObjectLookup.by_name('Ticket'), o_id: ticket.id).any? } + .to(false) + end + + it 'deletes all related Tags on destroy' do + create_list(:tag, 3, o: ticket) + + expect { ticket.destroy } + .to change { Tag.exists?(tag_object_id: Tag::Object.lookup(name: 'Ticket').id, o_id: ticket.id) } + .to(false) + end + + it 'deletes all related Histories on destroy' do + create_list(:history, 3, o: ticket) + + expect { ticket.destroy } + .to change { History.exists?(history_object_id: History::Object.lookup(name: 'Ticket').id, o_id: ticket.id) } + .to(false) + end + + it 'deletes all related Karma::ActivityLogs on destroy' do + create_list(:'karma/activity_log', 3, o: ticket) + + expect { ticket.destroy } + .to change { Karma::ActivityLog.exists?(object_lookup_id: ObjectLookup.by_name('Ticket'), o_id: ticket.id) } + .to(false) + end + + it 'deletes all related RecentViews on destroy' do + create_list(:recent_view, 3, o: ticket) + + expect { ticket.destroy } + .to change { RecentView.exists?(recent_view_object_id: ObjectLookup.by_name('Ticket'), o_id: ticket.id) } + .to(false) + end + + context 'when ticket is generated from email (with attachments)' do + subject(:ticket) { Channel::EmailParser.new.process({}, raw_email).first } + let(:raw_email) { File.read(Rails.root.join('test', 'data', 'mail', 'mail001.box')) } + + it 'adds attachments to the Store{::File,::Provider::DB} tables' do + expect { ticket } + .to change { Store.count }.by(2) + .and change { Store::File.count }.by(2) + .and change { Store::Provider::DB.count }.by(2) + end + + context 'and subsequently destroyed' do + it 'deletes all related attachments' do + ticket # create ticket + + expect { ticket.destroy } + .to change { Store.count }.by(-2) + .and change { Store::File.count }.by(-2) + .and change { Store::Provider::DB.count }.by(-2) + end + end + + context 'and a duplicate ticket is generated from the same email' do + before { ticket } # create ticket + let(:duplicate) { Channel::EmailParser.new.process({}, raw_email).first } + + it 'adds duplicate attachments to the Store table only' do + expect { duplicate } + .to change { Store.count }.by(2) + .and change { Store::File.count }.by(0) + .and change { Store::Provider::DB.count }.by(0) + end + + context 'when only the duplicate ticket is destroyed' do + it 'deletes only the duplicate attachments' do + duplicate # create ticket + + expect { duplicate.destroy } + .to change { Store.count }.by(-2) + .and change { Store::File.count }.by(0) + .and change { Store::Provider::DB.count }.by(0) + end + end + + context 'when only the duplicate ticket is destroyed' do + it 'deletes all related attachments' do + duplicate.destroy + + expect { ticket.destroy } + .to change { Store.count }.by(-2) + .and change { Store::File.count }.by(-2) + .and change { Store::Provider::DB.count }.by(-2) + end + end + end end end end diff --git a/test/unit/ticket_article_store_empty.rb b/test/unit/ticket_article_store_empty.rb deleted file mode 100644 index a07059015..000000000 --- a/test/unit/ticket_article_store_empty.rb +++ /dev/null @@ -1,74 +0,0 @@ -require 'test_helper' - -class TicketArticleStoreEmpty < ActiveSupport::TestCase - - test 'check if attachments are deleted after ticket is deleted' do - - current_count = Store.count - current_file_count = Store::File.count - current_backend_count = Store::Provider::DB.count - - email_raw_string = File.read(Rails.root.join('test', 'data', 'mail', 'mail001.box')) - ticket, article, user, mail = Channel::EmailParser.new.process({}, email_raw_string) - - next_count = Store.count - next_file_count = Store::File.count - next_backend_count = Store::Provider::DB.count - - assert_equal(current_count, next_count - 2) - assert_equal(current_file_count, next_file_count - 2) - assert_equal(current_backend_count, next_backend_count - 2) - - ticket.destroy! - - after_count = Store.count - after_file_count = Store::File.count - after_backend_count = Store::Provider::DB.count - - assert_equal(current_count, after_count) - assert_equal(current_file_count, after_file_count) - assert_equal(current_backend_count, after_backend_count) - - end - - test 'check if attachments are deleted after ticket same ticket 2 times is deleted' do - - current_count = Store.count - current_file_count = Store::File.count - current_backend_count = Store::Provider::DB.count - - email_raw_string = File.read(Rails.root.join('test', 'data', 'mail', 'mail001.box')) - ticket1, article1, user1, mail1 = Channel::EmailParser.new.process({}, email_raw_string) - ticket2, article2, user2, mail2 = Channel::EmailParser.new.process({}, email_raw_string) - - next_count = Store.count - next_file_count = Store::File.count - next_backend_count = Store::Provider::DB.count - - assert_equal(current_count, next_count - 4) - assert_equal(current_file_count, next_file_count - 2) - assert_equal(current_backend_count, next_backend_count - 2) - - ticket1.destroy! - - next_count = Store.count - next_file_count = Store::File.count - next_backend_count = Store::Provider::DB.count - - assert_equal(current_count, next_count - 2) - assert_equal(current_file_count, next_file_count - 2) - assert_equal(current_backend_count, next_backend_count - 2) - - ticket2.destroy! - - after_count = Store.count - after_file_count = Store::File.count - after_backend_count = Store::Provider::DB.count - - assert_equal(current_count, after_count) - assert_equal(current_file_count, after_file_count) - assert_equal(current_backend_count, after_backend_count) - - end - -end