NMS.GEN.LOG.007 - Location record history tracking (not including collections)

[larubbio]

NMS system shall maintain history of all changes done in a given Location record via code.

Tracking of changes via UI is tracked in #475

Tracking of collections field is tracked in #506

[larubbio]

Currently there is an issue in MDS that results in a StackOverflow when modifying our classes with recordHistory turned on.

https://applab.atlassian.net/browse/MOTECH-1713

[larubbio]

@kamknitt If you get the change tracking working for #137 #377 this ticket should also get the same solution.

[ksz-ksz]

@larubbio There are some issues with classloading during annotations processing after applying solution from #474. The good news is that Sebastian already have a fix for that and he promised to push it to the master tomorrow. Meanwhile, I have one question. What do we do with fields that are collections or maps? Do we want to track them too? I'm not sure how to handle this.

[larubbio]

@kamknitt It doesn't look like we use Map in the domain package so I think we can ignore that for this project.

Do we use collections anyplace other than in relationships? For example State -> District. In State we have a collection of Districts, and in District we have a StateId. I'm ok with us not recording a change in State when a District is added to the collection as long as we record the change in District (that StateId went from to State.id)

[ksz-ksz]

@larubbio You are right, I've gone too far with the map ;) So the tracked change of the relation reverse side covers the case of one-to-many relationships (if the Data Nucleus will trigger the changes tracking advice), but what with the many-to-many, for example State <-> Circle?

[larubbio]

Good point.... I think we would need to track that. Is there a way to simplify things for collections? Instead of showing previous and new value just record a note that an item was added or removed? We probably can't do that since those calls would be made on the collection.

I guess for now we can just record it as a limitation and see if it is acceptable.

On Mon, Jul 13, 2015 at 11:43 AM, kamknitt notifications@github.com wrote:

@larubbio https://github.com/larubbio You are right, I've gone too far with the map ;) So the tracked change of the relation reverse side covers the case of one-to-many relationships (if the Data Nucleus will trigger the changes tracking advice), but what with the many-to-many, for example State <-> Circle?

— Reply to this email directly or view it on GitHub https://github.com/motech-implementations/mim/issues/139#issuecomment-121019424 .

[larubbio]

@kamknitt Is it possible to split this out? So the tracking for non-collection fields can be submitted while the work for collection fields is ongoing?

[frankhuster]

Using the toString() old/new value for maps isn't sufficient?

On Wed, Jul 15, 2015 at 11:16 AM, Rob LaRubbio notifications@github.com wrote:

@kamknitt https://github.com/kamknitt Is it possible to split this out? So the tracking for non-collection fields can be submitted while the work for collection fields is ongoing?

— Reply to this email directly or view it on GitHub https://github.com/motech-implementations/mim/issues/139#issuecomment-121701776 .

Frank Huster Mobile Health Innovations Grameen Foundation +1.206.325.6690 x201

[larubbio]

I think this issue is that we can't trap the call to modify the collection, so we don't know if we need to call toString or not.

On Thu, Jul 16, 2015 at 8:05 AM, Frank Huster notifications@github.com wrote:

Using the toString() old/new value for maps isn't sufficient?

On Wed, Jul 15, 2015 at 11:16 AM, Rob LaRubbio notifications@github.com wrote:

@kamknitt https://github.com/kamknitt Is it possible to split this out? So the tracking for non-collection fields can be submitted while the work for collection fields is ongoing?

— Reply to this email directly or view it on GitHub < https://github.com/motech-implementations/mim/issues/139#issuecomment-121701776

.

Frank Huster Mobile Health Innovations Grameen Foundation +1.206.325.6690 x201

— Reply to this email directly or view it on GitHub https://github.com/motech-implementations/mim/issues/139#issuecomment-121983724 .

[ksz-ksz]

I've created a pull request containing location data changes tracking without the tracking of collections (relations). I have an idea to make it work for collections too. I'm thinking about wrapping the collection getter call with another AOP advice that would modify the getter return value. Instead of returning the real collection, the modified getter will be returning the decorated version of it. That decorated version will delegate the method calls to the underlying collection (the real one) and also it will be tracking changes in methods like add, remove, clear, etc. It would allows us to track only items that was actually added/removed. So instead of having change records like:

state([1,2,3], [1,2,4])

we will have:

state(added{4},removed{3})

or an abbreviated version:

state(A{4},R{3})

What do you think?

[larubbio]

Sounds good. I'm assuming it would also handle apis like addAll. Would you also handle a call to setStates that passes in a new collection?

-Rob

On Fri, Jul 17, 2015 at 6:11 AM, kamknitt notifications@github.com wrote:

I've created a pull request containing location data changes tracking without the tracking of collections (relations). I have an idea to make it work for collections too. I'm thinking about wrapping the collection getter call with another AOP advice that would modify the getter return value. Instead of returning the real collection, the modified getter will be returning the decorated version of it. That decorated version will delegate the method calls to the underlying collection (the real one) and also it will be tracking changes in methods like add, remove, clear, etc. It would allows us to track only items that was actually added/removed. So instead of having change records like:

state([1,2,3], [1,2,4])

we will have:

state(added{4},removed{3})

or an abbreviated version:

state(A{4},R{3})

What do you think?

— Reply to this email directly or view it on GitHub https://github.com/motech-implementations/mim/issues/139#issuecomment-122271851 .