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

Feature/log json attributes #601

Merged
merged 11 commits into from
Oct 6, 2019

Conversation

ReeceM
Copy link
Contributor

@ReeceM ReeceM commented Oct 1, 2019

fixes #274

This feature will add the functionality to add son sub-key filtering to the $logAttributes

It also ignores changes to the rest of the son object if it is changed and not in the logAttributes. This also works for the dirtyOnly setting, it won't save a change if that value didn't experience an update.

When the attribute is stored, I went with just keeping the key as it would be defined by a user.

i.e. if it is json->subkey->data it will be

[
    "attributes" => [
        "json->subkey->data" => 'value',
     ],
]

I think this would just be consistent to what is defined in the Model, and also less overhead changing it to something odd or getting mixed up with model relations.

This is related to the issue #274 where other discussion occurred before this PR.

I have added tests for the changes of this feature. Also updated the documentation to reflect the addition.

Please let me know what to improve.

Copy link
Collaborator

@Gummibeer Gummibeer left a comment

Choose a reason for hiding this comment

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

Thanks for your PR! 😊 Looks good at all, some small change requests.

And one thing to discuss:
In #594 we discuss the ability to revert. The current -> separator would complicate this. By using the dot-notation and Arr::set() we could easily create the array structure the key represents. This would make restoring it easier. Any downsides or problems? 🤔

src/Traits/DetectsChanges.php Outdated Show resolved Hide resolved
src/Traits/DetectsChanges.php Outdated Show resolved Hide resolved
src/Traits/DetectsChanges.php Outdated Show resolved Hide resolved
src/Traits/DetectsChanges.php Outdated Show resolved Hide resolved
@ReeceM
Copy link
Contributor Author

ReeceM commented Oct 2, 2019

Hi @Gummibeer, thanks for the suggestions and direction on that. I will implement the fixes/changes requested :).

The only thing I think with using the dot notation would be making sure the keys are assumed to be model relations.

But I think that can be accounted for by using the checks in place and reversing the logic

@Gummibeer
Copy link
Collaborator

I would just like to discuss the array structure part. This is again out of this scope because I'm fine with the current solution, we don't offer restoring it. And it's not this hard to solve in the possible restore method. I also tend to keep -> now because otherwise it will be hard to merge with the whole JSON.

@ReeceM
Copy link
Contributor Author

ReeceM commented Oct 2, 2019

Okay, so I think I may not be getting the point you are asking, I just got confused ☹️.

This is what I am understanding:

  • Don't stress about accounting for restoring now.
  • Keeping the -> in keys

It would be better for me to use that Arr::set() as it would result in better data structure, if that is what you were saying about merging the JSON data.

So instead of json->some->key it would be

[ "..." => [
      'json' => [
             'some' => [
                     'key' => 'data'
             ]
      ]
]

This would be easier to account for, not just for restoring, but just in the practical sense. Please let me know if that is better and what you are asking for.

Sorry if I'm bugging with questions.

@Gummibeer
Copy link
Collaborator

You are right, also in daily life the real array structure will be easier to handle.

  • Don't care about restoring
  • Use Arr:set() with dot-notation to create real array structure

Thanks for your work and no problem, better ask than doing it twice. 😉

@Gummibeer
Copy link
Collaborator

I will check it in detail until friday. But on mobile it looks good! 💙

@Gummibeer
Copy link
Collaborator

PS: can you fix StyleCI?

@ReeceM
Copy link
Contributor Author

ReeceM commented Oct 2, 2019

@Gummibeer, I can fix the StyleCI

tests/DetectsChangesTest.php Show resolved Hide resolved
docs/advanced-usage/logging-model-events.md Outdated Show resolved Hide resolved
docs/advanced-usage/logging-model-events.md Show resolved Hide resolved
@Gummibeer Gummibeer added enhancement hacktoberfest https://hacktoberfest.digitalocean.com labels Oct 4, 2019
ReeceM and others added 4 commits October 4, 2019 12:44
Changes to multiple levels are tracked
Tests added are for deep sub keys in array json

Also the the way it affects the changed data
@ReeceM ReeceM requested a review from Gummibeer October 4, 2019 20:22
@Gummibeer Gummibeer merged commit a28239b into spatie:master Oct 6, 2019
@Gummibeer
Copy link
Collaborator

https://github.com/spatie/laravel-activitylog/releases/tag/3.9.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement hacktoberfest https://hacktoberfest.digitalocean.com
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support for logging attribute change in json fields
2 participants