search

robwilkerson / CakePHP-Audit-Log-Plugin

Records changes made to an object during CRUD operations.
104 stars 78 forks source link

Enhancement: HABTM should also log additional changes #109

Open fr0z3nfyr opened 7 years ago

fr0z3nfyr commented 7 years ago

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?

ravage84 commented 7 years ago

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

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

xhs345 commented 7 years ago

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 commented 7 years ago

@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. :disappointed:

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

  1. attach Auditable to my SalesOrder and ProductsSalesOrder models
  2. change my forms to use ProductsSalesOrder.0.product_id, ProductsSalesOrder.0.description, ...
  3. 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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

How about:

fr0z3nfyr commented 7 years ago

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

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 commented 7 years ago

@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 commented 7 years ago

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

  1. Attach Auditable to Product, SalesOrder and ProductSalesOrder Model
  2. 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 commented 7 years ago

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 commented 7 years ago

@xhs345 @ravage84 can you please throw some light on afterAuditProperty() https://github.com/robwilkerson/CakePHP-Audit-Log-Plugin/blob/3596685137bb85b13f796c2ec62dade427595d9e/Model/Behavior/AuditableBehavior.php#L264-L271

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 commented 7 years ago

@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. https://github.com/robwilkerson/CakePHP-Audit-Log-Plugin/commit/59ca094df5f6af2d9439d9e7a722a5a3614b6457#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 commented 7 years ago

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 commented 7 years ago

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.

fr0z3nfyr commented 7 years ago

@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 commented 7 years ago

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