Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MARLIN-621] Do not send previously sent immutable entities to Connec!™ #186

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions .rubocop_todo.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ Metrics/AbcSize:
# Configuration parameters: CountComments.
Metrics/ClassLength:
Max: 110
Exclude:
- "app/models/maestrano/connector/rails/services/data_consolidator.rb"

# Offense count: 9
Metrics/CyclomaticComplexity:
Expand Down
3 changes: 2 additions & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,5 @@ rvm:
services:
- redis-server
before_install:
- gem install bundler # Update bundler version
- rvm @global do gem uninstall bundler -a -x
- rvm @global do gem install bundler -v 1.17.3
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@ def public_name(formatted_names)

(names[0..-2].join(', ') + " and #{names.last}").humanize
end

def immutable?
false
end
end

# input : {
Expand Down
4 changes: 4 additions & 0 deletions app/models/maestrano/connector/rails/concerns/entity.rb
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,10 @@ def singleton?
false
end

def immutable?
false
end

# Entity name in Connec!
def connec_entity_name
raise 'Not implemented'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ def consolidate_external_entities(external_entities, connec_entity_name)
entity_id = @current_entity.class.id_from_external_entity_hash(entity)
idmap = @current_entity.class.find_or_create_idmap(external_id: entity_id, organization_id: @organization.id, connec_entity: connec_entity_name.downcase)

next if @current_entity.class.immutable? && idmap.connec_id.present?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment above and a blank line after.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps extract this to a public method on the Entity class (?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its better to extract it into private method in the same class. It doesn't belong in Entity class as it also queries idmap.

# Not pushing entity to Connec!
next unless idmap.to_connec

Expand Down
14 changes: 14 additions & 0 deletions lib/generators/connector/templates/entity.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,23 +34,37 @@ def update_external_entity(mapped_connec_entity, external_id, external_entity_na
# e.g entity['id']
def self.id_from_external_entity_hash(entity)
# TODO
# This method return the id from external_entity_hash
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you duplicated the doc here? See #183

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ya my bad, rebase conflict was resolved wrong, fixed it

# e.g entity['id']
end

# This method return the last update date from an external_entity_hash
# e.g entity['last_update']
def self.last_update_date_from_external_entity_hash(entity)
# TODO
# This method returns the last update date from external_entity_hash
# e.g entity['last_update']
end

# This method return the creation date from an external_entity_hash
# e.g entity['created_at']
def self.creation_date_from_external_entity_hash(entity)
# TODO
# This method returns the creation date from external_entity_hash
# e.g entity['created_at']
end

# This method return true is entity is inactive in the external application
# e.g entity['status'] == 'INACTIVE'
def self.inactive_from_external_entity_hash?(entity)
# TODO
# This method return true if entity is inactive in the external application
# e.g entity['status'] == 'INACTIVE'
end

def self.immutable?(entity)
# TODO
# This method return true if entity is immutable.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method documentation should be above the method (see #183)

# An entity is immutable, if it is only ever creates, but never updated. eg. BankTransaction
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grammar.

An entity is immutable if it is only ever created but never updated

end
end
111 changes: 111 additions & 0 deletions spec/models/services/data_consolidator_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
require 'spec_helper'

def process_entities_to_cmp(entities)
entities.flat_map {|e| e[:entity]["id"] = e[:entity]["id"].first["id"]; e[:entity]}
end

describe Maestrano::Connector::Rails::Services::DataConsolidator do
let!(:organization) {create(:organization, uid: 'cld-123')}
let(:external_entities) {
[
{
"amount" => 8.93,
"type" => "RECEIVE",
"public_note" => "",
"transaction_date" => "2018-07-31 12:00:00 +0000",
"name" => "INTEREST PAYMENT",
"id" => "TX8934077941420180731",
"account_id" => "4518736"
},
{
"amount" => -26.47,
"type" => "SPEND",
"public_note" => "DISBURSEMENT TO MORTGAGOR",
"transaction_date" => "2018-01-16 12:00:00 +0000",
"name" => "DISBURSEMENT TO MORTGAGOR",
"id" => "201801160",
"account_id" => "4518738"
},
{
"amount" => 25.0,
"type" => "RECEIVE",
"public_note" => "LATE CHARGE PAID",
"transaction_date" => "2018-01-12 12:00:00 +0000",
"name" => "LATE CHARGE PAID",
"id" => "201801121",
"account_id" => "4518738"
}
]
}

class BankTransactionMapper
extend HashMapper
map from('name'), to('name')
map from('transaction_date'), to('transaction_date')
map from('type'), to('type')
map from('public_note'), to('public_note')
map from('account_id'), to('account_id')
map from('id'), to('id')
map from('lines'), to('lines')
map from('status'), to('status')
map from('amount'), to('amount')
end

class BankTransaction < Maestrano::Connector::Rails::Entity
def self.id_from_external_entity_hash(entity)
entity["id"]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

single quotes

end

def self.connec_entity_name
'Transaction'
end

# Entity name in external system
def self.external_entity_name
'Transaction'
end

def self.object_name_from_external_entity_hash(entity)
self.id_from_external_entity_hash(entity)
end

def self.mapper_class
BankTransactionMapper
end
end

describe 'consolidate_external_entities' do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

describe '#consolidate_external_entities'

See [betterspecs|http://www.betterspecs.org/#describe]

it 'should return all external entities' do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not use should but the 3rd person in the present tense:

it 'returns all external entities'

See [betterspecs|http://www.betterspecs.org/#should]

transaction = BankTransaction.new(organization, nil, nil)
allow(BankTransaction).to receive(:immutable?).and_return(false)
data_consolidator = Maestrano::Connector::Rails::Services::DataConsolidator.new(organization, transaction, {})
entities = data_consolidator.consolidate_external_entities(external_entities, 'BankTransaction')
ent = process_entities_to_cmp(entities)
expect(ent).to eq(external_entities)
end

it 'should return some external entities' do
Maestrano::Connector::Rails::IdMap.create(external_id: "TX8934077941420180731", external_entity: 'transaction', organization_id: organization.id, connec_entity: 'banktransaction', connec_id: 1)
transaction = BankTransaction.new(organization, nil, nil)
allow(BankTransaction).to receive(:immutable?).and_return(true)
data_consolidator = Maestrano::Connector::Rails::Services::DataConsolidator.new(organization, transaction, {})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

used described_class

entities = data_consolidator.consolidate_external_entities(external_entities, 'BankTransaction')
ent = process_entities_to_cmp(entities)
expect(ent.count).to eq(2)
expect(ent[0]).to eq(external_entities[1])
expect(ent[1]).to eq(external_entities[2])
end

it 'should return empty array' do
Maestrano::Connector::Rails::IdMap.create(external_id: "TX8934077941420180731", external_entity: 'transaction', organization_id: organization.id, connec_entity: 'banktransaction', connec_id: 1)
Maestrano::Connector::Rails::IdMap.create(external_id: "201801160", external_entity: 'transaction', organization_id: organization.id, connec_entity: 'banktransaction', connec_id: 2)
Maestrano::Connector::Rails::IdMap.create(external_id: "201801121", external_entity: 'transaction', organization_id: organization.id, connec_entity: 'banktransaction', connec_id: 3)
transaction = BankTransaction.new(organization, nil, nil)
allow(BankTransaction).to receive(:immutable?).and_return(true)
data_consolidator = Maestrano::Connector::Rails::Services::DataConsolidator.new(organization, transaction, {})
entities = data_consolidator.consolidate_external_entities(external_entities, 'BankTransaction')
expect(entities.count).to eq(0)
end
end
end