Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Log Additional HABTM data #110

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fr0z3nfyr
Copy link

Feature discussion ref: #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
  • readme
    • documented new feature enhancment

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
@@ -115,7 +115,9 @@ This will create the `audits` and `audit_deltas` tables that will store each obj
Applying the `AuditableBehavior` to a model is essentially the same as applying any other CakePHP behavior. The behavior does offer a few configuration options:

- ``ignore`` An array of property names to be ignored when records are created in the deltas table.
- ``ignoreDeep`` Boolean `true`/`false` to ignore the above array of property names in **HABTM** and **join table** when records are created in deltas table. Defaults to false. If a property name exists in `ignore` as well as `deepLog`, it will still be ignored.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need that.
The whole HABTM thing is already an edge case use and two (or better said three) configuration settings for just that is too much.

Copy link
Author

@fr0z3nfyr fr0z3nfyr Dec 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I know. It is not used unless explicitly set to true. Just thought it would matter if you are desperate to limit the size of logged HABTM data, like I do. It seems I just overthought this, I could easily use deepLog to control it anyway (except that it will not ignore any properties of join table, which was my target here).

// Grab just the ID values and sort those.
$habtmIds = array_values($habtmIds);
sort($habtmIds);
// Grab just the ID values and sort those.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too much indentation for the next three lines.

- ``habtm`` An array of models that have a HABTM relationship with the acting model and whose changes should be monitored with the model. If the HABTM model is auditable in its own right, don't include it here. This option is for related models whose changes are _only_ tracked relative to the acting model.
- ``deepLog`` Boolean or Array of property names. Defaults to false. `true` will log all properties of **HABTM** and **join table**. Array form can be used like ``array('habtmField1', 'habtmField2', 'habtmField3', 'joinTableClassName')``, this is especially useful when you want to log detailed HABTM association with `unique` key set to `true`/`keepExisting` (check [docs](http://book.cakephp.org/2.0/en/models/associations-linking-models-together.html#hasandbelongstomany-habtm))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

array in lower case.

@@ -146,7 +148,9 @@ class AnotherModel extends AppModel {
public $actsAs = array(
'AuditLog.Auditable' => array(
'ignore' => array('active', 'name', 'updated'),
'ignoreDeep' => true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above about ignoreDeep.

}
}

if ($this->settings[$Model->alias]['ignoreDeep']) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See below about ignoreDeep.

'{n}.id',
'{n}.id'
);
if (isset($this->settings[$Model->alias]['deepLog']) && $this->settings[$Model->alias]['deepLog']) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we extract that part into a protected method, something with "deepLog"? Methods are already way too long.

if (isset($this->settings[$Model->alias]['deepLog']) && $this->settings[$Model->alias]['deepLog']) {
$habtmData = $data[$habtmModel];
if (is_array($this->settings[$Model->alias]['deepLog']) && !empty($this->settings[$Model->alias]['deepLog'])) {
// keep selected HABTM fields only
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpicking: Upper Case for the first letter after //

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Started sounding like a mod on http://english.stackexchange.com 😆 Will look into your recommendations.


$auditData[$Model->alias][$habtmModel] = implode(',', $habtmIds);
$auditData[$Model->alias][$habtmModel] = implode(',', $habtmIds);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too much indentation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants