Fix order activity timeline blank entries

[wychoong]

No description provided.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
lunar-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 12, 2024 1:29pm

[alecritson]

@wychoong Apparently this might not be needed anymore? is that the case?

[wychoong]

@wychoong Apparently this might not be needed anymore? is that the case?

no, this is required for a complete fix

->logExcept(array_merge(['updated_at'], static::$logExcept ?? [])) 
// this to support exclude additional fields, 
// for example Order have additional activity log for `status` so this exclude avoid 2 entries
->logOnlyDirty() 
// without this, if a record is updated without changes, it is still logged, 
// can test with `$taggable->parent->touch()`

[alecritson]

@wychoong Apparently this might not be needed anymore? is that the case?

no, this is required for a complete fix

->logExcept(array_merge(['updated_at'], static::$logExcept ?? [])) 
// this to support exclude additional fields, 
// for example Order have additional activity log for `status` so this exclude avoid 2 entries
->logOnlyDirty() 
// without this, if a record is updated without changes, it is still logged, 
// can test with `$taggable->parent->touch()`

I think my confusion is why we need to exclude the status column. Are we performing a manual log of the change and spatie is also logging the status change automatically? If so would it make more sense to move the manual logging and let Spatie do it's thing?

[wychoong]

Are we performing a manual log of the change and spatie is also logging the status change automatically?

yes, correct
https://github.com/lunarphp/lunar/blob/0.6/packages/core/src/Observers/OrderObserver.php

for Order model there is a trait and observer to log for changes

would it make more sense to move the manual logging and let Spatie do it's thing

Not really. I think the confusion probably is in lunar context, this logging is for activity, not auditing the record values.
So a generic create/update can be just "Created"/"Updated" entry in activity feed. While certain changes can be logged seperately.

for example Order status

without excluding the status in this case, it will have 2 entries

[alecritson]

Would this be better a method call, just thinking then it will likely appease the phpstan warning and be a bit more flexible in the long run?

[wychoong]

Are you foreseeing a setter in register? Since it’s a static I don’t think it matters.

How would you like to see the implementation? A method to return an array or setter and getter with property?

[alecritson]

I think just a method to return an array should be enough, at least if it's on the trait it's easy to override.

[wychoong]

I've updated the implementation to use a getter and setter approach so that:

• lunar core can define default excluded field
• user can add their own fields with

        Order::addActivitylogExcept('another_field');

this gives the flexibility to user add their own exclusion so that they can register their own logging and log feed renderer

[glennjacobs]

@wychoong assuming this is still needed, could you target 0.7 branch please?

[wychoong]

@wychoong assuming this is still needed, could you target 0.7 branch please?

done

[alecritson]

I realise this has been hanging around for a while but it looks like there are some PHPStan issues to sort

[wychoong]

I realise this has been hanging around for a while but it looks like there are some PHPStan issues to sort

Thanks for sorting it out. Been busy lately