Skip to content

Conversation

@denydias
Copy link
Contributor

@denydias denydias commented Nov 11, 2020

Although the Activity model holds a description field, this field is used in index and search. If it holds many detailed information about the activity being logged, it could break index layout and add additional stress upon search.

To avoid that, a detail field is added. This new, optional field can hold any additional information required to better describe the activity. This is particularly useful to provide a fully functional auditing log as one can now add details for the entire data changed by the activity while keeping description as narrow as possible.

As the field is optional, the ActivityLogger trait could be used as usual, or include a second parameter with all the desired details to be logged. No additional configuration is required for this to work.

I'm not a native speaker of any of the supported languages in this package, but I've included translations for them. Fell free to change that as needed.

I hope this PR find its way to master. Thanks again for the great package, @jeremykenedy.

@denydias
Copy link
Contributor Author

denydias commented Nov 11, 2020

It looks that Scrutinizer has issues not related to this PR, hence failed check.

PHP Fatal error:  Uncaught PHP_CodeSniffer\Exceptions\RuntimeException:
  "continue" targeting switch is equivalent to "break". Did you mean to use "continue 2"?
  in /home/scrutinizer/.analysis/phpcs/vendor/squizlabs/php_codesniffer/src/Files/File.php on line 1279
  in /home/scrutinizer/.analysis/phpcs/vendor/squizlabs/php_codesniffer/src/Runner.php:557

StyleCI and Travis went fine tho.

Copy link
Owner

@jeremykenedy jeremykenedy left a comment

Choose a reason for hiding this comment

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

All looks great on your end.

In reviewing the code I see that I should probably make the description nullable with validator rules and a migration to update that. All on me to do that.

@denydias
Copy link
Contributor Author

All looks great on your end.

Nice!

In reviewing the code I see that I should probably make the description nullable with validator rules and a migration to update that. All on me to do that.

Already did that! See here for validator and here for migration. 😉

Thanks for reviewing and considering merging this, @jeremykenedy!

@denydias
Copy link
Contributor Author

Hi, @jeremykenedy! I wish all is well with you and your family.

I don't want to rush you, but do you intend to integrate this PR? If this is not ask too much, can you provide a time frame for it? (nothing precise, of course...)

Is there something I could change in the code that may help you?

@jeremykenedy jeremykenedy merged commit 06b5fa6 into jeremykenedy:master Dec 18, 2020
@denydias
Copy link
Contributor Author

Thank you so much, @jeremykenedy!

I'm looking forward to contribute to you soon.

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