Enhancement: HABTM should also log additional changes #109

fr0z3nfyr · 2016-11-17T05:57:29Z

In case of HABTM model, the behavior should also log any changes made to additional fields in the HABTM join table.

For example, a SalesOrder hasAndBelongsToMany Product while the structure of products_sales_orders may be something like:

Column	Type	Null
id	int(11)	No
sales_order_id	int(11)	No
product_id	int(11)	No
description	text	No
unit_price	float(8,2)	No
discount	float(4,2)	No
quantity	int(11)	No
created	datetime	No
created_by	int(11)	No
modified	datetime	Yes
modified_by	int(11)	Yes

and relationship setting be like:

public $hasAndBelongsToMany = array(
	'Product' => array(
		'className' => 'Product',
		'joinTable' => 'products_sales_orders',
		'foreignKey' => 'sales_order_id',
		'associationForeignKey' => 'product_id',
		'unique' => 'keepExisting',
		'conditions' => '',
		'fields' => '',
		'order' => '',
		'limit' => '',
		'offset' => '',
		'finderQuery' => '',
	),
	...	// more habtm relations
);

One would definitely want to log any changes made to the associated products' extra fields like description, quantity, discount etc. Can you think of a way to achieve this with existing setup? Is the suggested enhancement too specific or difficult to include in plugin?

The text was updated successfully, but these errors were encountered:

ravage84 · 2016-11-17T10:10:40Z

I agree, but without having looked at the code, I'm wondering why it does not do that already. 😕

That should certainly be possible.
Could you track it down in your setup?
That would greatly help.

xhs345 · 2016-11-21T20:13:56Z

Right now the plugin only checks the IDs in HABTM models (see: https://github.com/robwilkerson/CakePHP-Audit-Log-Plugin/blob/master/Model/Behavior/AuditableBehavior.php#L335).

Changing the association to Has Many through should work more reliable and it's the recommended setup for a structure like yours.

Changing the HABTM code in the plugin to your use-case is probably not trivial, just because of the different unique options

fr0z3nfyr · 2016-11-30T09:08:06Z

@ravage84 I guess @xhs345 comment answers your question. I tried modifying the plugin code to do that for me and ended up braking the plugin itself, so I reverted. 😞

@xhs345 I agree with your suggestion and can give it a try (have never before used hasMany through). So, just to clarify, if I perform all of these, the plugin should log the changes to both models?:

attach Auditable to my SalesOrder and ProductsSalesOrder models
change my forms to use ProductsSalesOrder.0.product_id, ProductsSalesOrder.0.description, ...
do a saveAll() operation on SalesOrder

I'm sure it will save my data but I doubt it will log any changes(audit) in ProductsSalesOrder because that is not the model which will be referred by $Model and the plugin doesn't take care of the changes made to hasMany models. Can you help?

Meanwhile, I'll try and find out if my assumptions are correct.

fr0z3nfyr · 2016-11-30T10:44:08Z

OK. Here's the update -
Instead of attempting the suggestion offered by @xhs345 I modified the plugin using the hint given by him (Thanks!).

I replaced https://github.com/robwilkerson/CakePHP-Audit-Log-Plugin/blob/master/Model/Behavior/AuditableBehavior.php#L369-L379 with $audit_data[$Model->alias][$habtm_model] = json_encode($data[$habtm_model]); and it all seemed to work for me as I wanted(almost). Although, this saves a lot of data (that's why I said "almost"), I think this is a good start for me.

Let me know if there is a possibility of accepting a PR, I can push the update. It will be easy, I will just check for an additional element, say (bool) $settings['habtm'][fullLog] in setup() and act accordingly to either save full change set or just the ids. Any suggestions?

ravage84 · 2016-11-30T10:56:48Z

@fr0z3nfyr it's not "my" repo, but I think the plugin should make it possible to save the additional data in HABTM associations. Personally I often use such a setup, too.

I guess the best way in this case is a config option that let's the developer decide in each case how much data he wants to save.
Something like saveAdditionalHABTMdata that can be set when attaching the behvaior, which can be true/false, where false is the default, as it would be the same as the current behavior.

Can you display an example of how much data is saved now? We shouln't save too much data, though.

fr0z3nfyr · 2016-11-30T11:18:11Z

Well, the data is a lot, see -
Audit record:

{
  "PurchaseOrder": {
    "id": "1",
    "parent_id": null,
    "lft": "1",
    "rght": "2",
    "po_number": "PO\/2015\/09\/1\/1",
    "order_date": "2016-09-09",
    "sales_order_id": "1",
    "currency_id": "61",
    "vendor_id": "2",
    "branch_id": "1",
    "ship_to": "1",
    "delivery_address": "Some address\r\nSome area\r\nSome city\r\nSome PIN",
    "recipient": "Jane Doe",
    "recipient_phone": "9999999999",
    "shipment_paid_by": "Supplier",
    "due_date": "2015-09-15",
    "internal_notes": "Testing",
    "supplier_notes": "Supplier doesn't have access to this portal yet",
    "terms": "1. Payment terms - 100% advance before delivery\r\n2. Delivery 2-3 days\r\n3. Packing will be in brown cardboard box bubble wrapped\r\n4. Shipment Invoice must match PO terms",
    "authorizer": "1",
    "po_status_id": "2",
    "cancellation_reason": "",
    "created": "2015-09-11 15:30:07",
    "created_by": "1",
    "modified": "2016-11-30 16:20:22",
    "modified_by": "1",
    "delete_status": false,
    "deleted_date": "0000-00-00 00:00:00",
    "company_id": "1",
    "Product": "[{\"id\":\"1\",\"name\":\" \\t  Outdoor 5 Megapixel HD Network Bullet  camera with 50m IR\",\"product_category_id\":null,\"brand_id\":\"1\",\"product_code\":\"PL-7274R\",\"description\":\"1. Feature 1\\r\\n2. Feature 2\\r\\n3. Feature 3\",\"assembled\":false,\"cost\":\"6000.00\",\"hs_code\":\"12345\",\"created\":\"2016-04-25 13:55:55\",\"created_by\":\"1\",\"modified\":\"2016-04-30 11:29:40\",\"modified_by\":\"1\",\"company_id\":\"1\",\"product_name\":\"PL-7274R ( \\t  Outdoor 5 Megapixel HD Network Bullet  camera with 50m IR)\",\"PoItem\":{\"id\":\"41\",\"purchase_order_id\":\"1\",\"item_code\":\"DC-1221\",\"product_id\":\"1\",\"description\":\"\",\"unit_price\":\"6000.00\",\"discount\":\"0.00\",\"quantity\":\"5\",\"received_quantity\":\"0\",\"created\":\"2016-11-30 16:20:22\",\"created_by\":\"1\",\"modified\":\"2016-11-30 16:20:22\",\"modified_by\":null,\"company_id\":\"0\"}},{\"id\":\"3\",\"name\":\" Back Box\",\"product_category_id\":\"6\",\"brand_id\":\"1\",\"product_code\":\"PL-0601-SPV\",\"description\":\"1. Feature 1\\r\\n2. Feature 2\\r\\n3. Feature 3\\r\\n4. Feature 4\",\"assembled\":false,\"cost\":\"40.00\",\"hs_code\":\"12232\",\"created\":\"2016-04-25 14:03:14\",\"created_by\":\"1\",\"modified\":\"2016-04-25 14:03:14\",\"modified_by\":null,\"company_id\":\"1\",\"product_name\":\"PL-0601-SPV ( Back Box)\",\"PoItem\":{\"id\":\"42\",\"purchase_order_id\":\"1\",\"item_code\":\"BB-5545\",\"product_id\":\"3\",\"description\":\"\",\"unit_price\":\"35.00\",\"discount\":\"0.00\",\"quantity\":\"1500\",\"received_quantity\":\"0\",\"created\":\"2016-11-30 16:20:22\",\"created_by\":\"1\",\"modified\":\"2016-11-30 16:20:22\",\"modified_by\":null,\"company_id\":\"0\"}}]"
  }
}

P.S: Ohh! there I realized that the HABTM data is json encoded twice, solution is to encode it in afterSave function rather than where I was doing. So,
$audit_data[$Model->alias][$habtm_model] = json_encode($data[$habtm_model]); should be $audit_data[$Model->alias][$habtm_model] = $data[$habtm_model]; and in afterSave(), $delta will become:

		$delta = array(
			'AuditDelta' => array(
				'property_name' => $property,
				'old_value'     => json_encode($this->_original[$Model->alias][$property]),
				'new_value'     => json_encode($value)
			)
		);

Of course, again based on the config setting from setup (like you mentioned too). I'll fix it at my end.

UPDATE: Corrected data for HABTM in Audit after above change will be

"Product": [
  {
    "id": "1",
    "name": " \t  Outdoor 5 Megapixel HD Network Bullet  camera with 50m IR",
    "product_category_id": null,
    "brand_id": "1",
    "product_code": "PL-7274R",
    "description": "1. Feature 1\r\n2. Feature 2\r\n3. Feature 3",
    "assembled": false,
    "cost": "6000.00",
    "hs_code": "12345",
    "created": "2016-04-25 13:55:55",
    "created_by": "1",
    "modified": "2016-04-30 11:29:40",
    "modified_by": "1",
    "company_id": "1",
    "product_name": "PL-7274R ( \t  Outdoor 5 Megapixel HD Network Bullet  camera with 50m IR)",
    "PoItem": {
      "id": "41",
      "purchase_order_id": "1",
      "item_code": "DC-1221",
      "product_id": "1",
      "description": "",
      "unit_price": "6000.00",
      "discount": "0.00",
      "quantity": "5",
      "received_quantity": "0",
      "created": "2016-11-30 16:20:22",
      "created_by": "1",
      "modified": "2016-11-30 16:20:22",
      "modified_by": null,
      "company_id": "0"
    }
  },
  {
    "id": "3",
    "name": " Back Box",
    "product_category_id": "6",
    "brand_id": "1",
    "product_code": "PL-0601-SPV",
    "description": "1. Feature 1\r\n2. Feature 2\r\n3. Feature 3\r\n4. Feature 4",
    "assembled": false,
    "cost": "40.00",
    "hs_code": "12232",
    "created": "2016-04-25 14:03:14",
    "created_by": "1",
    "modified": "2016-04-25 14:03:14",
    "modified_by": null,
    "company_id": "1",
    "product_name": "PL-0601-SPV ( Back Box)",
    "PoItem": {
      "id": "42",
      "purchase_order_id": "1",
      "item_code": "BB-5545",
      "product_id": "3",
      "description": "",
      "unit_price": "35.00",
      "discount": "0.00",
      "quantity": "1500",
      "received_quantity": "0",
      "created": "2016-11-30 16:20:22",
      "created_by": "1",
      "modified": "2016-11-30 16:20:22",
      "modified_by": null,
      "company_id": "0"
    }
  }
]

For AuditDelta (old_value):

[
  {
    "id": "1",
    "name": " \t  Outdoor 5 Megapixel HD Network Bullet  camera with 50m IR",
    "product_category_id": null,
    "brand_id": "1",
    "product_code": "PL-7274R",
    "description": "1. Feature 1\r\n2. Feature 2\r\n3. Feature 3",
    "assembled": false,
    "cost": "6000.00",
    "hs_code": "12345",
    "created": "2016-04-25 13:55:55",
    "created_by": "1",
    "modified": "2016-04-30 11:29:40",
    "modified_by": "1",
    "company_id": "1",
    "product_name": "PL-7274R ( \t  Outdoor 5 Megapixel HD Network Bullet  camera with 50m IR)",
    "PoItem": {
      "id": "39",
      "purchase_order_id": "1",
      "item_code": "DC-1221",
      "product_id": "1",
      "description": "",
      "unit_price": "6000.00",
      "discount": "0.00",
      "quantity": "1",
      "received_quantity": "0",
      "created": "2016-11-30 16:04:56",
      "created_by": "1",
      "modified": "2016-11-30 16:04:56",
      "modified_by": null,
      "company_id": "0"
    }
  },
  {
    "id": "3",
    "name": " Back Box",
    "product_category_id": "6",
    "brand_id": "1",
    "product_code": "PL-0601-SPV",
    "description": "1. Feature 1\r\n2. Feature 2\r\n3. Feature 3\r\n4. Feature 4",
    "assembled": false,
    "cost": "40.00",
    "hs_code": "12232",
    "created": "2016-04-25 14:03:14",
    "created_by": "1",
    "modified": "2016-04-25 14:03:14",
    "modified_by": null,
    "company_id": "1",
    "product_name": "PL-0601-SPV ( Back Box)",
    "PoItem": {
      "id": "40",
      "purchase_order_id": "1",
      "item_code": "BB-5545",
      "product_id": "3",
      "description": "",
      "unit_price": "35.00",
      "discount": "0.00",
      "quantity": "1500",
      "received_quantity": "0",
      "created": "2016-11-30 16:04:56",
      "created_by": "1",
      "modified": "2016-11-30 16:04:56",
      "modified_by": null,
      "company_id": "0"
    }
  }
]

Pretty much same size of data for new_value if I don't add/remove products.

Note that these are just 2 products in 1 PurchaseOrder, in general case, there can be any number of HABTM entries, imagine the data size if there were 50...!!

fr0z3nfyr · 2016-11-30T11:22:11Z

Now, thinking of going in reverse direction, instead of setting a boolean config, setting an array of specific columns from each habtm to be logged in setup. that will simplify things a bit. If no columns are set, it will log only HABTM ids else it will log only the requested columns. Let's see how that comes out!

ravage84 · 2016-11-30T11:40:56Z

@fr0z3nfyr thanks. Can you reformat the whole data with four backticks and proper indentation? It's barely readable like this. 😼

How about:

false = no additional fields (default, current behavior)
true = all fields
array of fields = only these fields

fr0z3nfyr · 2016-11-30T12:46:16Z

Updated - formatted JSON in code blocks and with proper indentation. 😌

I normally avoid formatting JSON to limit "vertical scroll" on post, if the string is very long. Take a look at http://jsonviewer.stack.hu/ it can format JSON very efficiently.

About false/true/Array approach - looks reasonable to me. I can then do something like

if($habtmExtras) {
	if(is_array($habtmExtras)) {
		// save only selected HABTM fields
	} else {
		// save all HABTM fields
	}
} else {
	// save only HABTM id(s)
}

Now, I'm also wondering whether there should be a way to set fields from both habtm table and join table or automatically save all columns from join table if $habtmExtras is not false. I think it should be latter to keep the initialization configuration simple. In every case, any column listed in ignore array will/should be ignored for $Model, HABTM table as well as join table. Makes sense? I'll anyway make a PR after I complete this, let's wait to find out if it can be accepted.

ravage84 · 2016-11-30T13:03:39Z

@fr0z3nfyr if data in such a post isn't readable, then it's losing its point 😼
Thanks for reformatting and the link. Now I see the problem with the amount of data much clearer.
Alternatively you can post such data dumps to a gist or data dumper site and link to it.

Anyway, I agree we shouldn't overdo with the configuration. But since we are about to add more functionality, I prefer to do it in a flexible way.

But I guess, let's hear @xhs345's opinion on that.

xhs345 · 2016-11-30T19:19:02Z

Wow, lots of discussions happening all of a sudden. I like it :)

@fr0z3nfyr

@xhs345 I agree with your suggestion and can give it a try (have never before used hasMany through). So, just to clarify, if I perform all of these, the plugin should log the changes to both models?:
attach Auditable to my SalesOrder and ProductsSalesOrder models change my forms to use ProductsSalesOrder.0.product_id, ProductsSalesOrder.0.description, ... do a saveAll() operation on SalesOrder
I'm sure it will save my data but I doubt it will log any changes(audit) in ProductsSalesOrder because that is not the model which will be referred by $Model and the plugin doesn't take care of the changes made to hasMany models. Can you help?

Almost. The 'typical way' would be

Attach Auditable to Product, SalesOrder and ProductSalesOrder Model
Use the View that belongs to the ProductSalesOrder Model and in the form use ProductSalesOrder.product_id, ProductSalesOrder.description, etc.

It's explained in more detail in the manual here. I tested it locally and it worked fine for me

xhs345 · 2016-11-30T19:33:00Z

Of course I'm also happy if you could get it to work for HABTM associations and create a PR for it.

And thank you @ravage84 for leading the discussion and all your input

fr0z3nfyr · 2016-12-01T07:26:07Z

@xhs345 @ravage84 can you please throw some light on afterAuditProperty()

CakePHP-Audit-Log-Plugin/Model/Behavior/AuditableBehavior.php

Lines 264 to 271 in 3596685

    
           if (!$created && $Model->hasMethod('afterAuditProperty')) { 
        
           	$Model->afterAuditProperty( 
        
           		$Model, 
        
           		$delta['AuditDelta']['property_name'], 
        
           		$this->_original[$Model->alias][$delta['AuditDelta']['property_name']], 
        
           		$delta['AuditDelta']['new_value'] 
        
           	); 
        
           }

I just realized that I can use afterAuditProperty() function in my model and do some fancy stuff, only that I wonder why call the function for every updated property instead of building an array of updated properties and calling it just once outside foreach.

Anyways, my work on this issue is in progress, will soon make a PR.

ravage84 · 2016-12-01T16:48:51Z

@xhs345 you are welcome!

@fr0z3nfyr afterAuditProperty() is an event callback. Unfortunately they are not documented yet, as explained here. Personally haven't used them.

Those event callbacks seem to date back to 2010, when the initial commit was made by Rob. 59ca094#diff-a5728d8bda7296e6cd5a6907a113b619R182

I wonder, if we could even rip those out. That would mean, though, we would need to release a 2.x version of the plugin...

@fr0z3nfyr if those event callbacks would be fired differently, would they be more useful to you?

xhs345 · 2016-12-01T18:47:59Z

I haven't used the callbacks either, but I can see their use. I think just adding some documentation for then should be a good strategy for now

fr0z3nfyr · 2016-12-02T05:23:59Z

I can think of cases where I'd want to trigger create/update/delete notifications to subscribed users or perform some other background tasks like updating inventory etc. I think I just posted hastily without checking afterAuditUpdate() which does what I mentioned earlier. I too believe documenting these callbacks would be good idea for now.

Feature discussion ref: robwilkerson#109 Added: * additinal config settings - ``ignoreDeep``: boolean; used to ignore specified properties in `ignore` array from HABTM and join table data - ``deepLog``: boolean/Array; used to log all/specified properties of HABTM and join table data Edited: * _getModelData() function - manipulate log data according to config new settings * afterSave() function - `json_encode()` new/old values of a property if deepLog is configured

fr0z3nfyr · 2016-12-02T08:10:01Z

@xhs345 created a PR #110 just now.

I have successfully tested it with my local test instance.

Though, I noticed few issues, not related specifically to this PR using with cake-2.6.3. The issue was same as mentioned in this comment. I found a way to fix it by defining $this->Audit = ClassRegistry::init('AuditLog.Audit'); before $Model->Audit->create(); and using $this->Audit->create();$this->Audit->save($data); instead of $Model->. Similarly for AuditDelta, it was $this->Audit->AuditDelta->save($delta);. Also, removed the binding of Audit to $Model (I anyway don't see a good reason to do this even if it works with latest versions of cake. Why is binding with $Model needed except for saving log?)

Another issue(not so much) I noticed was that the Audit doesn't ignore the specified properties in Audit.json_object (see code), though it did ignore these properties when it saved logs in AuditDelta as expected (duh!).

P.S: I can't develop feature in PR for cake 1.3 (I believe that 1.3 branch should be marked as deprecated anyway). I can look into 3.x branch but not immediately.

Thanks @xhs345 @ravage84 for all the discussion on this thread. Cheers!!

ravage84 · 2016-12-02T10:09:04Z

@fr0z3nfyr no need for any update on the CakePHP 1.x related branch. https://github.com/robwilkerson/CakePHP-Audit-Log-Plugin#cakephp-13x
Also no need to dig into any development for a 3.x version. There are alternative plugins already ready to be used. https://github.com/robwilkerson/CakePHP-Audit-Log-Plugin#cakephp-3x

Could you please create a separate issue detailing the probem with older CakePHP versions?
This way it will not get lost and we can work on that separately.

According to the documentation this seems to be intended:

/**
 *   - ignore array, optional
 *            An array of property names to be ignored when records
 *            are created in the deltas table.

when records are created in the deltas table.

I understand, you would want to ignore fields when adding records to the Audit table, too?
If so, please create a separate issue. We can have a look at it.

fr0z3nfyr mentioned this issue Dec 2, 2016

Log Additional HABTM data #110

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancement: HABTM should also log additional changes #109

Enhancement: HABTM should also log additional changes #109

fr0z3nfyr commented Nov 17, 2016 •

edited

Loading

ravage84 commented Nov 17, 2016

xhs345 commented Nov 21, 2016

fr0z3nfyr commented Nov 30, 2016 •

edited

Loading

fr0z3nfyr commented Nov 30, 2016 •

edited

Loading

ravage84 commented Nov 30, 2016

fr0z3nfyr commented Nov 30, 2016 •

edited

Loading

fr0z3nfyr commented Nov 30, 2016

ravage84 commented Nov 30, 2016

fr0z3nfyr commented Nov 30, 2016

ravage84 commented Nov 30, 2016 •

edited

Loading

xhs345 commented Nov 30, 2016

xhs345 commented Nov 30, 2016 •

edited

Loading

fr0z3nfyr commented Dec 1, 2016

ravage84 commented Dec 1, 2016

xhs345 commented Dec 1, 2016

fr0z3nfyr commented Dec 2, 2016

fr0z3nfyr commented Dec 2, 2016

ravage84 commented Dec 2, 2016

Enhancement: HABTM should also log additional changes #109

Enhancement: HABTM should also log additional changes #109

Comments

fr0z3nfyr commented Nov 17, 2016 • edited Loading

ravage84 commented Nov 17, 2016

xhs345 commented Nov 21, 2016

fr0z3nfyr commented Nov 30, 2016 • edited Loading

fr0z3nfyr commented Nov 30, 2016 • edited Loading

ravage84 commented Nov 30, 2016

fr0z3nfyr commented Nov 30, 2016 • edited Loading

fr0z3nfyr commented Nov 30, 2016

ravage84 commented Nov 30, 2016

fr0z3nfyr commented Nov 30, 2016

ravage84 commented Nov 30, 2016 • edited Loading

xhs345 commented Nov 30, 2016

xhs345 commented Nov 30, 2016 • edited Loading

fr0z3nfyr commented Dec 1, 2016

ravage84 commented Dec 1, 2016

xhs345 commented Dec 1, 2016

fr0z3nfyr commented Dec 2, 2016

fr0z3nfyr commented Dec 2, 2016

ravage84 commented Dec 2, 2016

fr0z3nfyr commented Nov 17, 2016 •

edited

Loading

fr0z3nfyr commented Nov 30, 2016 •

edited

Loading

fr0z3nfyr commented Nov 30, 2016 •

edited

Loading

fr0z3nfyr commented Nov 30, 2016 •

edited

Loading

ravage84 commented Nov 30, 2016 •

edited

Loading

xhs345 commented Nov 30, 2016 •

edited

Loading