Bank import: browse record is written to

[Stefan Rijnhart (Opener)]

in account_banking/wizard/bank_import.py, in _get_move_info() the browse record 'move_line' is being written to.

[Dimitri John Ledkov (xnox)]

hit the bug myself. reviewed & tested code. released.

[Stefan Rijnhart (Opener) (stefan-opener)]

Hi Dmitrijs,

Thanks for your action! To be honest there is a problem with the merged branch [1] and browse object caching. When the same browse object is read again, the old information, which is cached, is being returned. As the move_line browse object argument is potentially being reused later on, this could lead to problems.

Of course, the original code is wrong in writing an object id to a browse object place holder, but we should consider writing a reference to a newly created browse object of the reconciliation to move_line.reconciliation_id.

Should this merge be reverted for the above, note that this branch also fixes a separate problem with handling of an existing partial reconciliation.

Cheers,
Stefan.

[1] http://bazaar.launchpad.net/~therp-nl/banking-addons/fix-lp903929/revision/71

[Dimitri John Ledkov (ex-credativ) (dle-credativ)]

Hey Stefan,

Well the way I did it first in my branch locally (before noticing yours). Was to add:

  move_line = move_line.browse()

* by default browse record attributes are called with (cr,uid,self.id)
* above will 'refresh' the browse record

I saw that potentially it is being used, but somehow figured that you have avoid it somehow. Let me go through it again.

Yes, existing partial reconcilatation made me look for bugs on here =)

Regards.

[Stefan Rijnhart (Opener) (stefan-opener)]

Hi Dmitrijs,

although move_line = move_line.browse() does refresh the local move_line variable, it does so by creating a new browse record and updating the local reference. The browse record that the caller passed originally, and which may be processed again further down does not get updated. Instead, we would need to find a way to invalidate the cache on an existing browse record.

Looks like resetting _data might do the trick

move_line._data[move_line.id] = {'id': move_line.id} might do the trick.

Alternatively but less elegant, we may keep your suggestion and return the move_line back to the caller, changing its call to

move_line = link_payment(cr, uid, move_line, ...)

Cheers,
Stefan.

[Dimitri John Ledkov (ex-credativ) (dle-credativ)]

Hmmm.... in trunk browse_record class has refresh function which does what we want.
We can possibly backport this for our purposes =)

    def refresh(self):
        """Force refreshing this browse_record's data and all the data of the
           records that belong to the same cache, by emptying the cache completely,
           preserving only the record identifiers (for prefetching optimizations).
        """
        for model, model_cache in self._cache.iteritems():
            # only preserve the ids of the records that were in the cache
            cached_ids = dict([(i, {'id': i}) for i in model_cache.keys()])
            self._cache[model].clear()
            self._cache[model].update(cached_ids)

[Stefan Rijnhart (Opener) (stefan-opener)]

Hi Dmitrijs,

good find! Shall I do the monkey patching or do want to do it?

Cheers,
Stefan.

[Dimitri John Ledkov (ex-credativ) (dle-credativ)]

money patch sound good.

it looks like it's done?

[Stefan Rijnhart (Opener) (stefan-opener)]

Yes. Forgot to update this bug's status. Fixed by merging

https://code.launchpad.net/~banking-addons-team/banking-addons/6.0-lp942876-clear_browse_record_cache