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

SelectArrayGently plugin #658

Merged
merged 8 commits into from
Sep 1, 2021
Merged

SelectArrayGently plugin #658

merged 8 commits into from
Sep 1, 2021

Conversation

Antikon
Copy link
Contributor

@Antikon Antikon commented Aug 29, 2021

What are you changing/introducing

A new ngrest plugin which is similar to SelectArray plugin.

What is the reason for changing/introducing

The SelectArray plugin has bad behavior, which leads to a change in the model attribute (see #439). This behavior can be disabled by assignAfterFind option (#444). Howewer, this option disables autoconversion of data in a list view also.

SelectArrayGently plugin has the advantages of original SelectArray plugin and has not its disadvantages.

QA

Q A
Is bugfix? no
New feature? yes
Breaks BC? no
Tests pass? yes
Fixed issues #490

Copy link
Member

@nadar nadar left a comment

Choose a reason for hiding this comment

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

Hey @Antikon - thats a great change, thank you very much - even though the name "gently" is uncommon, i like it 👍

i just have added some phpdoc request changes, and please add a changelog. thanks!

src/ngrest/plugins/SelectArrayGently.php Show resolved Hide resolved
src/ngrest/plugins/SelectArrayGently.php Outdated Show resolved Hide resolved
@Antikon Antikon requested a review from nadar August 30, 2021 14:55
Copy link
Member

@nadar nadar left a comment

Choose a reason for hiding this comment

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

One thing, you write ...override the values from the database. but i think you mean that value of that attributes does not recieve "the value" and recieves the "key" instead? 😳

Or are you reffering to your example code new Query()->all()? Sorry for the delay

@Antikon
Copy link
Contributor Author

Antikon commented Aug 31, 2021

Sorry, I didn't get exactly what you mean.

Initial 'SelectArray' plugin changes the model's attributes values which came from DB using onAfterListFind method of abstract Select class. In the 'SelectArrayGently' plugin this behavior is disabled, so the values of the attributes remain unchanged. By ...override the values from the database I meant exactly that behavior.
Maybe the word default should be added as it says in the Guide.

@Antikon Antikon requested a review from nadar August 31, 2021 15:32
@nadar
Copy link
Member

nadar commented Sep 1, 2021

Ok, i see. I think my description in $assignAfterFind was chosen bad. Because the attribute will be overwritte with the label defined in $data array, which is not directly "the value from the database". https://github.com/luyadev/luya-module-admin/blob/master/src/ngrest/plugins/Select.php#L117

👍

Could we then not just make a new class like

class SelectArrayGently from SelectArray
{
   public $assignAfterFind = false;
}

Is there any difference to your code? Is the new Angular Directive Required?

@Antikon
Copy link
Contributor Author

Antikon commented Sep 1, 2021

Ок, let me explain once more.

Let's assume we have 'Country' and 'City' models.
In a last one we have a relation

public function getCountry()
    {
        return $this->hasOne(ConfCountries::class, ['id' => 'country_id']);
    }

We also have in 'City' model

public function ngRestAttributeTypes()
{
    'country_id' => [
        'selectArray',
        'data'       => ConfCountries::getCountriesArray(), // This is just example of array, e.g. [1 => 'France', '2' => 'Germany', etc]
    ],
}

country_id is listed ngRestScopes() of 'City' model.

SelectArray plugin renders a dropdown in create or update context. And shows country name in list context. This is what we expect from it. But at the same time in list context it changes the values of country_id attribute of City model (Was 1, now France). This is exactly what I call override the values from the database.

This behavior can be disabled by $assignAfterFind = false. In that case a dropdown still exist in create or update context, but in list context the country id instead of country name will be shown. In order to avoid this, I do the conversion in the SelectArrayGently plugin on the client side by new AngularJs directive.

Thus, the behavior of the SelectArrayGently plugin is the same as SelectArray plugin, but the initial value of the attribute (country_id in our example) remains unchanged.

@nadar
Copy link
Member

nadar commented Sep 1, 2021

In order to avoid this, I do the conversion in the SelectArrayGently plugin on the client side by new AngularJs directive.

👍

@nadar nadar merged commit c9ae0fa into luyadev:master Sep 1, 2021
@Antikon Antikon deleted the selectArray2 branch September 3, 2021 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants