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

hhemanth · 2019-01-07T05:49:30Z

If an entity is immutable, then we should not send again to connec if we have already sent it

Eg: BankTransaction, is an immutable entity, Once we see the transactions in Marlin Banking Adapter, it doesnot change
So we don't have to send the transactions again to Connec again, especially during a full_sync
If the full sync is configured to run every day and if we send all transcations to connec every time,
then connec gets overloaded and returns 500

Spec Output

Maestrano::Connector::Rails::Services::DataConsolidator
  #consolidate_external_entities
    entity is immutable and all entities have been pushed to connec! before
      return empty array
    entity is immutable and some entities have been pushed to connec! before
      returns some external entities
    entity is not immutable, no entity has been pushed to connec! before
      returns all external entities

… to connec if we have already sent it Eg: BankTransaction, is an immutable entity, Once we see the transactions in Marlin Banking Adapter, it doesnot change So we don't have to send the transactions again to Connec again, especially during a full_sync If the full sync is configured to run every day and if we send all transcations to connec every time, then connec gets overloaded and returns 500

travis-ci/travis-ci#8969 (comment)

ouranos

I'd change the specs structure with context to make it easier to understand.

Looking a the description (eg: rspec --format doc) I see:

Maestrano::Connector::Rails::Services::DataConsolidator
  #consolidate_external_entities
    return all external entities
    return some external entities
    returns empty array

We don't know why it's returning different output. so I'd use some context (with before blocks to create the records)

Other than the specs, the code LGTM

ouranos · 2019-01-07T06:40:43Z

app/models/maestrano/connector/rails/services/data_consolidator.rb

@@ -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?


Add a comment above and a blank line after.

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

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.

ouranos · 2019-01-07T06:42:19Z

lib/generators/connector/templates/entity.rb

@@ -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


I think you duplicated the doc here? See #183

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

ouranos · 2019-01-07T06:42:47Z

lib/generators/connector/templates/entity.rb

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


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

ouranos · 2019-01-07T06:43:36Z

lib/generators/connector/templates/entity.rb

+  def self.immutable?(entity)
+    # TODO
+    # This method return true if entity is immutable.
+    # An entity is immutable, if it is only ever creates, but never updated. eg. BankTransaction


Grammar.

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

ouranos · 2019-01-07T06:45:32Z

spec/models/services/data_consolidator_spec.rb

+  end
+
+  describe 'consolidate_external_entities' do
+    it 'should return all external entities' do


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

it 'returns all external entities'

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

ouranos · 2019-01-07T06:46:11Z

spec/models/services/data_consolidator_spec.rb

+    end
+  end
+
+  describe 'consolidate_external_entities' do


describe '#consolidate_external_entities'

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

ouranos · 2019-01-07T06:47:17Z

spec/models/services/data_consolidator_spec.rb

+      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, {})


used described_class

ouranos · 2019-01-07T06:52:55Z

Also please edit the title of the PR as it took the first commit message and truncated it

iseessel · 2019-01-07T18:44:17Z

.travis.yml

@@ -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


iseessel · 2019-01-07T18:50:15Z

spec/models/services/data_consolidator_spec.rb

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


single quotes

iseessel

overall, lgtm -- think this will be a big improvement. I Agree with Oli about clarifying the specs.

iseessel · 2019-01-07T18:55:57Z

I'm also still not 100% sure if this will fix all of our load problems.

The load issue seemed to be coming from a single batch call, with only 20 entities, so I think if there are more than 20 entities to sync it may still fail.

Obviously this is a great start. Will also need a code change in the banking-adapter too right?

Fixed specs, it was in bad shape. Fixed errors due to rebase conflicts. Added comment for next statement and refactored into a private method

ouranos

The specs could be tidied up to follow best practices. This is a public project and the one that most likely to be used for external developers so we need to be particulary attentive to the quality of the code.

I realised we don't use rubocop-rspec for this project. I'll add it to the list of further improvements

ouranos · 2019-01-08T04:39:53Z

spec/models/services/data_consolidator_spec.rb

+
+    context 'entity is not immutable, no entity has been pushed to connec! before' do
+      it 'returns all external entities' do
+        allow(BankTransaction).to receive(:immutable?).and_return(false)


This should be in the context, eg:

context 'entity is mutable' do before { allow(BankTransaction).to receive(:immutable?).and_return(false) } ... end

This will make it easier to read and is good practice if you have multiple tests (only one in this case)

ouranos · 2019-01-08T04:41:00Z

spec/models/services/data_consolidator_spec.rb

+
+    context 'entity is immutable and some entities have been pushed to connec! before' do
+      it 'returns 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)


Same for the creation and the stub. The context setup should be in a before block.
The it blocks should be quite small

Especially since the immutable is repeated for the next step.

So something like this:

context 'when entity is immutable' do before { allow(BankTransaction).to receive(:immutable?).and_return(true) } context 'when some entities have already been pushed to connec!' do before do Maestrano::Connector::Rails::IdMap.create(external_id: 'TX8934077941420180731', external_entity: 'transaction', organization_id: organization.id, connec_entity: 'banktransaction', connec_id: 1) end it 'returns some external entities' do# Or it 'only returns the new entities' ... # Only 1 expectation here which is clearer than the 3 separates expect(ent).to match_array(external_entities[1], external_entities[2]) end end context 'when all entities have already been pushed to connec!' do before do ... end it 'does not return any entities' end end

ouranos · 2019-01-09T02:34:27Z

spec/models/services/data_consolidator_spec.rb

+        it 'only returns new entities' do
+          # expect(subject.count).to eq(2)
+          expect(subject[0]).to eq(external_entities[1])
+          expect(subject[1]).to eq(external_entities[2])


match_array didn't work?
In that case, you should still have an expectation on the count. Otherwise a rotation of the entities will pass the specs ;)

Organizing examples within contexts Using before for stubs and data setups

iseessel · 2019-01-09T18:20:05Z

@ouranos @hhemanth I have a major worry about this strategy.

Unenroll purge and per-app purge do not delete the idmap fields. This means that if we unenroll and re-enroll, banking data won't get synced again. In order to fix this scenario we would need to add this purge.

Because changes in batch size alone, still works, I think we need to implement the idmap purge before we can use this strategy.

EDIT: This is NOT currently an issue because unenroll scopes to a new organization and per-app purge does not include ofx data

iseessel · 2019-01-23T18:28:35Z

I'm not convinced this is needed. See: https://maestranoteam.slack.com/archives/GECNLHD5K/p1547168285051100

# Discards entities that do not need to be pushed because
# they have not been updated since their last push

https://github.com/maestrano/maestrano-connector-rails/blob/master/app/models/maestrano/connector/rails/concerns/entity.rb#L434`` (edited)

 def not_modified_since_last_push_to_connec?(idmap, entity)
   not_modified = idmap.last_push_to_connec && idmap.last_push_to_connec > @current_entity.class.last_update_date_from_external_entity_hash(entity)
   Maestrano::Connector::Rails::ConnectorLogger.log('info', @organization, "Discard #{Maestrano::Connector::Rails::External.external_name} #{@current_entity.class.external_entity_name} : #{entity}") if not_modified
   not_modified
 end

https://github.com/maestrano/maestrano-connector-rails/blob/master/app/models/maestrano/connector/rails/services/data_consolidator.rb#L103 (edited)

def self.last_update_date_from_external_entity_hash(entity)
  if entity['code'] == 'forecast'
    # use Time.current for forecast transaction so that external entity can preempt connec entity
    Time.current
  else
    entity['transaction_date'] ? Time.zone.parse(entity['transaction_date']) : Time.current
  end
end

https://github.com/maestrano/marlin-banking-adapter/blob/master/app/models/entities/bank_transaction.rb#L28 (edited)

Note, this idMap checking is bypassed if opts[:full_sync], which I think is appropriate behavior, as we want to be able to bypass this in case of data integrity issues.

I think thus, the idea of immutability is baked into this already as for immutable objects the updated_at in general, and the transaction_date in specific won't ever change @hhemanth @ouranos @alexmaestrano @alexmaestrano thoughts?

iseessel · 2019-01-23T18:38:49Z

app/models/maestrano/connector/rails/services/data_consolidator.rb

@@ -81,6 +81,9 @@ 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)

+        # Don't push to connec, the immutable entities which have already been pushed
+        next if immutable_and_already_pushed_to_connec?(idmap)


I personally think this is already baked into behavior (see my main comments).

If we are convinced we need the explicitness of this immutability flag, then I would prefer to do:

next if immutable_and_already_pushed_to_connec?(idmap) && !opts[:full_sync]

or

next if immutable_and_already_pushed_to_connec?(idmap) && !opts[:forced]

or even

next if immutable_and_already_pushed_to_connec?(idmap) && !opts[:forced] && !opts[:full_sync]

So we have the option to send immutable entities to quickbooks anyway in case of data integrity issues.

iseessel

IMO, don't think this is necessary.

If convinced it is, we should have the ability to disable this skip.

hhemanth force-pushed the feature/MARLIN-621-full-sync-optimisation branch from 7ed659b to cd7ab61 Compare January 7, 2019 05:57

hhemanth force-pushed the feature/MARLIN-621-full-sync-optimisation branch from cd7ab61 to 6709211 Compare January 7, 2019 06:01

Fixing travis ci build

131c05d

travis-ci/travis-ci#8969 (comment)

hhemanth requested review from iseessel and ouranos January 7, 2019 06:36

ouranos requested review from adamaziz15 and alexmaestrano January 7, 2019 06:39

ouranos added the enhancement label Jan 7, 2019

ouranos reviewed Jan 7, 2019

View reviewed changes

iseessel reviewed Jan 7, 2019

View reviewed changes

iseessel suggested changes Jan 7, 2019

View reviewed changes

hhemanth changed the title ~~[MARLIN-621] If an entity is immutable, then we should not send again…~~ [MARLIN-621] PR to not send entities to connec which are sent already and are immutable Jan 7, 2019

[MARLIN-621] addressing review comments

703672e

Fixed specs, it was in bad shape. Fixed errors due to rebase conflicts. Added comment for next statement and refactored into a private method

ouranos reviewed Jan 8, 2019

View reviewed changes

ouranos changed the title ~~[MARLIN-621] PR to not send entities to connec which are sent already and are immutable~~ [MARLIN-621] Do not send previously sent immutable entities to Connec!™ Jan 8, 2019

ouranos reviewed Jan 9, 2019

View reviewed changes

[MARLIN-621] fixing rspec inline with best practices

3f78eb7

Organizing examples within contexts Using before for stubs and data setups

hhemanth force-pushed the feature/MARLIN-621-full-sync-optimisation branch from 8bb1802 to 3f78eb7 Compare January 9, 2019 03:18

iseessel reviewed Jan 23, 2019

View reviewed changes

iseessel suggested changes Jan 23, 2019

View reviewed changes

ouranos added the wontfix label Apr 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hhemanth commented Jan 7, 2019 •

edited

ouranos left a comment

ouranos Jan 7, 2019

iseessel Jan 7, 2019

hhemanth Jan 7, 2019

ouranos Jan 7, 2019

hhemanth Jan 7, 2019

ouranos Jan 7, 2019

ouranos Jan 7, 2019

ouranos Jan 7, 2019

ouranos Jan 7, 2019

ouranos Jan 7, 2019

ouranos commented Jan 7, 2019

iseessel Jan 7, 2019

iseessel Jan 7, 2019

iseessel left a comment

iseessel commented Jan 7, 2019 •

edited

ouranos left a comment

ouranos Jan 8, 2019

ouranos Jan 8, 2019

ouranos Jan 9, 2019

iseessel commented Jan 9, 2019 •

edited

iseessel commented Jan 23, 2019 •

edited

iseessel Jan 23, 2019 •

edited

iseessel left a comment

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

Are you sure you want to change the base?

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

Conversation

hhemanth commented Jan 7, 2019 • edited

ouranos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ouranos commented Jan 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iseessel left a comment

Choose a reason for hiding this comment

iseessel commented Jan 7, 2019 • edited

ouranos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iseessel commented Jan 9, 2019 • edited

iseessel commented Jan 23, 2019 • edited

iseessel Jan 23, 2019 • edited

Choose a reason for hiding this comment

iseessel left a comment

Choose a reason for hiding this comment

hhemanth commented Jan 7, 2019 •

edited

iseessel commented Jan 7, 2019 •

edited

iseessel commented Jan 9, 2019 •

edited

iseessel commented Jan 23, 2019 •

edited

iseessel Jan 23, 2019 •

edited