Laravel-Backpack / revise-operation

An admin interface for venturecraft/revisionable - audit log for your Eloquent entries.
Other
42 stars 10 forks source link

[Bug]Translatable revisions #19

Open pxpm opened 2 years ago

pxpm commented 2 years ago

WHY

BEFORE - What was wrong? What was happening before this PR?

RevisionOperation didn't take into account translatable attributes so it would attempt to save the whole translations inside a single translation key when restoring. Reported in: Laravel-Backpack/revise-operation#20

AFTER - What is happening after this PR?

The translations are re-created from the old saved translations and manually assigned.

HOW

How did you achieve that, in technical terms?

Manually set back the translation when restoring the entry attribute

Is it a breaking change or non-breaking change?

I don't think it's a breaking change, this was not properly working after all.

How can we test the before & after?

Save a value for a translatable attribute that is using revisions, try to restore that value.

welcome[bot] commented 2 years ago

BOOM! Your first PR with us, thank you so much! Someone will take a look at it shortly.

Please keep in mind that:

Thank you!

-- Justin Case The Backpack Robot

pxpm commented 2 years ago

@tabacitu i picked up @guleswine idea from Laravel-Backpack/CRUD#16 of parsing the translatable fields. I did that behind a configuration because some people might want to see the db raw value.

We could improve this by parsing translations as default behaviour and having a link or something that on click display the raw json or something like that.

tabacitu commented 2 years ago

@pxpm I just tried testing this, but it didn't work for me. It calls upon a forgetTranslations() method that doesn't exist.

Here's how I tested:

Result - 4 things now show up as having been changed - name, details, features and extras: CleanShot 2022-05-03 at 09 18 09

Which is unfortunate but... let's focus on the name for now (bottom of the page). If I click "Undo" on that, it will show up a bubble notification, saying that the forgetTranslations() method doesn't exist on the model:

https://user-images.githubusercontent.com/1032474/166411812-9c78148d-959f-4fdb-a054-28d7235402e8.mp4

I looked through CRUD in the hope that a PR there will have this forgetTranslations() method but no. Looked through RevisionableTrait... no. No idea how this is supposed to work.

tabacitu commented 2 years ago

PS. This PR seemed a lot cleaner - https://github.com/Laravel-Backpack/revise-operation/pull/16 - what does PR 19 do, that PR 16 doesn't do, that justifies the extra complexity? That PR didn't work in my tests either, it throws an error when I load the "revisions" page - https://flareapp.io/share/xmN0WL37#F61

guleswine commented 1 year ago

I found a missed case in my PR, when inside the translated value, contains an object of attributes

your commit should be something like

@if(is_object($value))
    <p style="margin-bottom: 0px; padding-bottom:0px;"><b>{{ strtoupper($locale) }}</b>: {{json_encode($value)}}</p>
@else
    <p style="margin-bottom: 0px; padding-bottom:0px;"><b>{{ strtoupper($locale) }}</b>: {{$value}}</p>
@endif