Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]Translatable revisions #19

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[Bug]Translatable revisions #19

wants to merge 3 commits into from

Conversation

pxpm
Copy link
Contributor

@pxpm pxpm commented Apr 8, 2022

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: #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
Copy link

welcome bot commented Apr 8, 2022

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

Please keep in mind that:

  • if this constitutes a breaking change, it might take quite a while for this to get merged; we try to emulate the Laravel release cycle as much as possible, so developers can upgrade both software once; this means a new big release every ~6 months;
  • even if it's a non-breaking change, it might take a few days/weeks for the PR to get merged; unless it's a no-brainer, we like to have some community feedback on new features, before we merge them; this leads to higher-quality code, in the end; we learnt this the hard way :-)
  • not all PRs get merged; sometimes we just have to hold out new features, to keep the packages lean; sometimes we don't include features that only apply to niche use cases;
  • we're not perfect; if you think we're wrong, call us out on it; but in a kind way :-) we all make mistakes, best we learn from them and build better software together;

Thank you!

--
Justin Case
The Backpack Robot

@pxpm pxpm added the bug Something isn't working label Apr 8, 2022
@pxpm
Copy link
Contributor Author

pxpm commented Apr 8, 2022

@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.

@pxpm pxpm changed the title [Bug] Translatable revisions [Bug][WIP] Translatable revisions Apr 11, 2022
@pxpm pxpm changed the title [Bug][WIP] Translatable revisions [Bug]Translatable revisions Apr 11, 2022
@tabacitu tabacitu assigned pxpm and unassigned tabacitu May 3, 2022
@tabacitu
Copy link
Member

tabacitu commented May 3, 2022

@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:

  • I added RevisionableTrait and identifiableAttribute() to the Product model;
  • I added the ReviseOperation trait to ProductCrudController;
  • I went to the Product list, edited a Product name to New title;

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:

CleanShot.2022-05-03.at.09.19.30.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
Copy link
Member

tabacitu commented May 3, 2022

PS. This PR seemed a lot cleaner - #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
Copy link

guleswine commented Jan 29, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Priority: SHOULD
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants