Closed lukasoppermann closed 8 years ago
I'd accept a PR with tests that adds a causer
scope to the package.
Also the database migration needs a little modify. It should be indexable by user_id and subject_id.
Also it would be nice to do as Taylor has done for the notifications migration table in Laravel 5.3: use string type of IDs as primary key instead of integers. This could be great in these types of tables as the number of rows could go wild and noSQL could be needed in the future. Using string type IDs could make that future migration easy as a breeze.
I'd accept a PR that adds that to the package.
On Tuesday, 6 September 2016, sullysheperd notifications@github.com wrote:
Also the database migration needs a little modify. It should be indexable by user_id and subject_id.
Also it would be nice to do as Taylor has done for the notifications migration table in Laravel 5.3: use string type of IDs as primary key instead of integers. This could be great in these types of tables as the number of rows could go wild and noSQL could be needed in the future. Using string type IDs could make that future migration easy as a breeze.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/spatie/laravel-activitylog/issues/58#issuecomment-244868679, or mute the thread https://github.com/notifications/unsubscribe-auth/AAdiDTM67rdImf_LR2V2QcK4twFloPEDks5qnRM7gaJpZM4Jtkxt .
Freek Van der Herten https://spatie.be +32 495 84 27 91
I am not at the point where I am implementing this package, but it will happen in the next weeks. I will send a PR for the causer
scope once I am there. I think the changes in the database should be a different PR? While I can see the causer
scope getting a performance benefit from it, it is not needed, can thus would be better in a separate, small PR @sullysheperd.
Send 2 seperate PR's yes.
I send a PR for the scopes.
@sullysheperd Please send a PR for your idea of how to modify the migrations yourself, as I am not sure what you exactly want to change, thanks.
@lukasoppermann to be honest, I'm not really good with Github, I'm not sure how PR works. Today I implemented the activitiy_logs in my application and here is the table schema:
$table->string('id')->primary();
$table->integer('subject_id')->index();
$table->string('subject_type')->index();
$table->string('name');
$table->integer('user_id')->index();
$table->ipAddress('ip_address');
$table->string('user_agent')->index();
$table->timestamps();
Notice those index() methods. Using indexes in database schema is a must do task, especially for these kind of cases. Imagine you have millions of rows in the table, commanding database to retrieve all the records of a user WILL take so long (as database has to search for that user_id in every single row) IF you don't use indexes. On the other hand, when you use indexes, the database sorts rows in based on it, so it doesn't have to search for the row, it just picks it up.
And about that string type primary key, it's extremely useful if you decide to migrate to noSQL in the future; and with a relational database, it doesn't harm. That's why Tylor has used it in notifications table that ships with Laravel 5.3.
Let me know if your need more explanation. Regards
Hey,
any chance you could add a method to the
Activity
model so that only logs for a specific causer are retrieved? May even with another one for the current user only?I know I can extend it, but I would think this could be a pretty useful thing that other people might use as well.