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.

Sign up for GitHub

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

Jump to bottom

Add methods getTranslationChanges() and wasTranslationChanged() #231

Closed
wants to merge 14 commits into from
Closed

Add methods getTranslationChanges() and wasTranslationChanged() #231

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
d50c17d
Prettified Code!
Tschoatscho Jun 10, 2021
bec641d
add methods getTranslationChanges and wasTranslationChanged to Transl…
Tschoatscho Jun 10, 2021
8d5326d
add dev dependency laravel/legacy-factories for laravel 8 compatibility
Tschoatscho Jun 10, 2021
5102892
fix typo in existing test
Tschoatscho Jun 10, 2021
777c337
add unit tests for new methods getTranslationChanges and wasTranslati…
Tschoatscho Jun 10, 2021
17b0148
Revert "add dev dependency laravel/legacy-factories for laravel 8 com…
Tschoatscho Jun 10, 2021
f2fbe38
Merge branch 'Astrotomic:main' into issue-229
Tschoatscho Jun 11, 2021
845e8f1
change output of Translatable::getTranslationChanges() to associative…
Tschoatscho Jun 21, 2021
0633a68
add new Translatable::wasTranslationChanged() with unit tests
Tschoatscho Jun 21, 2021
6986ffe
fix formatting issues for styleCI
Tschoatscho Jun 21, 2021
60d20de
fix formatting issues for styleCI (even more)
Tschoatscho Jun 21, 2021
8c635c2
fix formatting issues for styleCI (last!)
Tschoatscho Jun 21, 2021
6953156
fix formatting issues for styleCI (oh dear!)
Tschoatscho Jun 21, 2021
7163a88
Apply suggestions from code review
Tschoatscho Sep 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 65 additions & 0 deletions src/Translatable/Translatable.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,11 @@

use Astrotomic\Translatable\Traits\Relationship;
use Astrotomic\Translatable\Traits\Scopes;
use Astrotomic\Translatable\Validation\RuleFactory;
use Illuminate\Database\Eloquent\Collection;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\ModelNotFoundException;
use Illuminate\Support\Arr;
use Illuminate\Support\Str;

/**
Expand Down Expand Up @@ -458,4 +460,67 @@ public function __isset($key)
{
return $this->isTranslationAttribute($key) || parent::__isset($key);
}

public function getTranslationChanges(): array
{
$translationChanges = [];
foreach ($this->translations as $translation) {
$changes = $translation->getChanges();
if (! empty($changes)) {
$translationChanges[$translation->locale] = $changes;
}
}

return $translationChanges;
}

public function wasTranslationChanged($attributes = null, $locale = null): bool
Tschoatscho marked this conversation as resolved.
Show resolved Hide resolved
{
$translationChanges = $this->getTranslationChanges();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are a few cases missing and therefore the check isn't complete:

if empty($translationChanges) 
    return false; // absolutely nothing changed

if ($attributes === null && $locale === null) 
    return true; // something changed and the user doesn't filter anything

if (is_string($attributes) && $locale !== null)
    return isset($translationChanges[$locale][$attributes]); // specific translation changed

if ($attributes === null && $locale !== null) 
    return empty($translationChanges[$locale]); // check if anything for locale changed

if (is_string($attributes) && $locale === null) 
    return empty(array_column($translationChanges, $attributes)); // any translation of single attribute changed

// loop over matrix and do checks

if (empty($translationChanges)) {
return false;
}
if (empty($attributes)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (empty($attributes)) {
if ($attributes === null) {

return true;
}
if (! is_array($attributes)) {
$attributes = [$attributes];
}
Tschoatscho marked this conversation as resolved.
Show resolved Hide resolved
if (config('translatable.rule_factory.format') === RuleFactory::FORMAT_KEY) {
$attributes = array_map(function ($attribute) {
return implode('.', array_reverse(explode(':', $attribute)));
}, $attributes);
}
Comment on lines +491 to +495
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the user can provide attribute and locale I don't see a reason why we should do this "complex" string handling here. wasTranslationChanged('title', 'de') should check if the german title was changed. So it's easier to use/read and more clear than wasTranslationChanged('de.name') or wasTranslationChanged('name:de').
By removing this string exploding the following logic also gets a lot easier.


$attributesWithoutLocale = [];
foreach ($attributes as $attribute) {
if (Arr::get($translationChanges, $attribute)) {
return true;
}
if (! Str::contains($attribute, '.')) {
$attributesWithoutLocale[] = $attribute;
}
}
if (! empty($locale)) {
$localeChanges = Arr::get($translationChanges, $locale);
if (empty($localeChanges)) {
return false;
}
foreach ($attributesWithoutLocale as $attribute) {
if (isset($localeChanges[$attribute])) {
return true;
}
}
} else {
foreach ($attributesWithoutLocale as $attribute) {
foreach ($translationChanges as $localeChanges) {
if (isset($localeChanges[$attribute])) {
return true;
}
}
}
}

return false;
}
}
66 changes: 63 additions & 3 deletions tests/TranslatableTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
use Astrotomic\Translatable\Tests\Eloquent\Person;
use Astrotomic\Translatable\Tests\Eloquent\Vegetable;
use Astrotomic\Translatable\Tests\Eloquent\VegetableTranslation;
use Astrotomic\Translatable\Validation\RuleFactory;
use Illuminate\Database\Eloquent\MassAssignmentException;
use Illuminate\Database\Eloquent\ModelNotFoundException;
use Illuminate\Support\Facades\App;
Expand Down Expand Up @@ -438,10 +439,10 @@ public function it_returns_if_attribute_is_translated(): void
/** @test */
public function config_overrides_apps_locale(): void
{
$veegtable = factory(Vegetable::class)->create(['name:de' => 'Erbsen']);
$vegetable = factory(Vegetable::class)->create(['name:de' => 'Erbsen']);
Gummibeer marked this conversation as resolved.
Show resolved Hide resolved
App::make('config')->set('translatable.locale', 'de');

static::assertEquals('Erbsen', $veegtable->name);
static::assertEquals('Erbsen', $vegetable->name);
}

/** @test */
Expand Down Expand Up @@ -768,7 +769,8 @@ public function numeric_translated_attribute(): void
$this->app->make('config')->set('translatable.fallback_locale', 'de');
$this->app->make('config')->set('translatable.use_fallback', true);

$vegetable = new class extends Vegetable {
$vegetable = new class extends Vegetable
{
protected $table = 'vegetables';
public $translationModel = VegetableTranslation::class;

Expand Down Expand Up @@ -1058,4 +1060,62 @@ public function it_can_restore_translations_in_a_transaction()
$this->assertDatabaseHas('vegetables', ['identity' => $vegetable->identity]);
$this->assertDatabaseHas('vegetable_translations', ['vegetable_identity' => $vegetable->identity]);
}

/** @test */
public function it_returns_no_translation_changes_for_unchanged_model()
Tschoatscho marked this conversation as resolved.
Show resolved Hide resolved
{
$vegetable = factory(Vegetable::class)->create([
'en' => ['name' => 'Peas'],
'de' => ['name' => 'Erbsen'],
]);
$changes = $vegetable->getTranslationChanges();
static::assertIsArray($changes);
static::assertEmpty($changes);
}

/** @test */
public function it_returns_translation_changes_for_changed_model()
Tschoatscho marked this conversation as resolved.
Show resolved Hide resolved
{
$vegetable = factory(Vegetable::class)->create([
'en' => ['name' => 'Peas'],
'de' => ['name' => 'Birnen'],
]);
$vegetable->fill(['de' => ['name' => 'Erbsen']]);
$vegetable->save();

static::assertEquals(['de' => ['name' => 'Erbsen']], $vegetable->getTranslationChanges());

$this->app->make('config')->set('translatable.rule_factory.format', RuleFactory::FORMAT_KEY);
static::assertEquals(['de' => ['name' => 'Erbsen']], $vegetable->getTranslationChanges());
}

/** @test */
public function it_returns_whether_any_translation_has_changed()
Tschoatscho marked this conversation as resolved.
Show resolved Hide resolved
{
$vegetable = factory(Vegetable::class)->create([
'en' => ['name' => 'Peas'],
'de' => ['name' => 'Birnen'],
]);
static::assertFalse($vegetable->wasTranslationChanged());
$vegetable->fill(['de' => ['name' => 'Erbsen']]);
$vegetable->save();
static::assertTrue($vegetable->wasTranslationChanged());
static::assertTrue($vegetable->wasTranslationChanged(['name']));
static::assertTrue($vegetable->wasTranslationChanged('name'));
static::assertTrue($vegetable->wasTranslationChanged('name', 'de'));
static::assertFalse($vegetable->wasTranslationChanged('name', 'en'));
static::assertTrue($vegetable->wasTranslationChanged('de.name'));
static::assertTrue($vegetable->wasTranslationChanged('de.name'), 'en');
static::assertFalse($vegetable->wasTranslationChanged('en.name'));
static::assertFalse($vegetable->wasTranslationChanged('name:de'));

$this->app->make('config')->set('translatable.rule_factory.format', RuleFactory::FORMAT_KEY);
static::assertTrue($vegetable->wasTranslationChanged('name'));
static::assertTrue($vegetable->wasTranslationChanged('name', 'de'));
static::assertFalse($vegetable->wasTranslationChanged('name', 'en'));
static::assertTrue($vegetable->wasTranslationChanged('name:de'));
static::assertFalse($vegetable->wasTranslationChanged('name:en'));
static::assertTrue($vegetable->wasTranslationChanged('de.name'));
static::assertFalse($vegetable->wasTranslationChanged('en.name'));
}
}