search

Laravel-Backpack / activity-log

MIT License
23 stars 6 forks source link

To have or not to have our own Trait #18

Closed promatik closed 1 year ago

promatik commented 1 year ago

Maybe we need to rethink this. Previously, we thought of making the package plug-and-play. Install it and that's it. But it looks like we can't actually make it plug-and-play. Devs will still need to make all of their models use a LogActivity trait.

So let's keep in mind the use cases for this package:

  • 60% - a developer who needs to start tracking admin activity is installing this, and wants some or all of their Models tracked;
  • 20% - a developer who is already using spatie/activity-log wants an interface for it; they are already logging stuff - maybe model changes, maybe custom activities;
  • 5% - a developer who is unhappy with Revisionable is installing this, as an alternative;

What I'm saying is... maybe it's best to do as little custom stuff in the Model as we can, and let the developer use the Spatie package. Then... we only provide an interface for that package.

We are unhappy with Spatie's default logging. I know. Personally I would have loved to have an option to just enable logging for all models, all attributes, only dirty. Without having to add the trait. But... it is what it is. I think it's easier for us to maintain... and for the developer to understand... if we just instruct them to add the Spatie trait and Spatie needed function to their models. Our recommandation is to LogOptions::defaults()->logAll()->logOnlyDirty(); but if they want to do something else... fine... our interface will still work and be useful.

That way, using the Trait and defining what gets logged... isn't a part of installing our package. It's a requirement of using Spatie/ActivityLog. Which is actually true.

_Originally posted by @tabacitu in https://github.com/Laravel-Backpack/activity-log/pull/8#discussion_r1308417975_

tabacitu commented 1 year ago

Ok fine we keep it for now. It does make developer's lives easier, having a default.

But I propose we make the default logAll() instead of logFillable().

promatik commented 1 year ago

Did it in https://github.com/Laravel-Backpack/activity-log/pull/8/commits/a62ec7073ec796f3fbf454d94b2d3bc9a28f3da5 šŸ™ŒšŸŽ‰