Added support for editable association fields from type choice in Lis… #4449

christingruber · 2017-04-11T20:27:23Z

I am targeting this branch, because this is new feature with BC.

Changelog

### Added
- Added editable support for association fields from type choice in `ListMapper`
- Added also new `class` option for field description

To do

Update the tests
Update the documentation

Subject

Added support for editable association fields from type choice in ListMapper, also added new field description option class. As example:

    protected function configureListFields(ListMapper $listMapper)
    {
        $listMapper
            ->add('status', 'choice', array(
                'editable' => true,
                'class' => 'Vendor\ExampleBundle\Entity\PageStatus',
                'choices' => array(
                    1 => 'Active',
                    2 => 'Inactive',
                    3 => 'Draft',
                ),
            ));
    }

…tMapper

greg0ire · 2017-04-12T08:11:29Z

Controller/HelperController.php

+                ->getAssociationNames();
+
+            if (!in_array($field, $associations)) {
+                return new JsonResponse(array(


Why not throw an exception instead ? This would be triggered by a programming error, right?

greg0ire · 2017-04-12T08:16:03Z

Controller/HelperController.php

+
+            if (!$value) {
+                return new JsonResponse(array(
+                    'status' => 'KO',


I was gonna say "use a status code, duh!", but then I saw that's what is being done in the whole class… anyway, please set the status code to 400.

Statuscode 400 for all KO responses in method setObjectFieldValueAction(), right? Or only for my patched part?

Maybe a 404 would be even better in your case.
For other responses it depends, but it's not your responsability (of course if you want to rework that, it will a very welcome change though). Some should use 405, others should use 404, and others should use 400. Then, client-side, the statusCode option of jQuery.ajax should be used instead of relying on this field status which contains… a code.

greg0ire

Wow, nice job!! Tiny changes needed…

greg0ire · 2017-04-12T15:59:42Z

Controller/HelperController.php

        $context = $request->get('context');

        $admin = $this->pool->getInstance($code);
        $admin->setRequest($request);

        // alter should be done by using a post method
        if (!$request->isXmlHttpRequest()) {
-            return new JsonResponse(array('status' => 'KO', 'message' => 'Expected a XmlHttpRequest request header'));
+            return new JsonResponse('Expected a XmlHttpRequest request header', 405);


"a" => "an" (the sound "ex" starts with a vowel)

greg0ire · 2017-04-12T16:01:21Z

Controller/HelperController.php

+
+            if (!$value) {
+                return new JsonResponse(sprintf('Edit failed, object with id "%s" not found in association "%s".',
+                        $originalValue, $field), 404);


This is not PSR-compliant, although not detected yet. Please put each sprintf argument on its own line.

greg0ire · 2017-04-12T16:04:00Z

Fix http status handling for json responses on editables

This is pedantic : it fixes no bug, and is not a visible improvement for the user (I think). If you agree, remove it from the changelog.

greg0ire · 2017-04-12T16:04:59Z

Controller/HelperController.php

-                'status' => 'KO',
-                'message' => 'The field cannot be edit, editable option must be set to true',
-            ));
+            return new JsonResponse('The field cannot be edit, editable option must be set to true', 400);


"edit" => "edited"

greg0ire · 2017-04-12T19:32:04Z

Controller/HelperController.php

        }

        if ($request->getMethod() != 'POST') {
-            return new JsonResponse('Expected a POST Request', 405);
+            return new JsonResponse('Expected an POST Request', 405);


Post starts with a consonant sound, so "a"

greg0ire · 2017-04-13T10:17:37Z

Can you add some tests here : https://github.com/sonata-project/SonataAdminBundle/blob/3.x/Tests/Controller/HelperControllerTest.php ? Could be merged anyway if you can't, couldn't it @sonata-project/contributors ?

greg0ire · 2017-04-24T12:51:00Z

@sonata-project/contributors please review

vworldat · 2017-05-03T10:48:25Z

@sonata-project/contributors how is this BC break compatible with semver? Tagging this as 3.17 broke compability with current SonataPageBundle (enabling/disabling blocks does not work anymore because the javascript in sonata-page.back.js is now outdated).

greg0ire · 2017-05-03T12:44:22Z

the javascript in sonata-page.back.js is now outdated

Can you elaborate? Where precisely is the BC-break?

vworldat · 2017-05-03T12:50:48Z

The javascript explicitly checks for the status and OK JSON response that has been removed, e.g. here: https://github.com/sonata-project/SonataPageBundle/blob/3.x/Resources/public/sonata-page.back.js#L589

I don't know if any other libraries depend on those responses. Changing API response syntax is a breaking change to me 🙂

greg0ire · 2017-05-03T12:55:40Z

how is this BC break compatible with semver

It clearly isn't, I just wasn't aware that was part of a public API. Would you kindly make a PR that would restore the array, but keeps the status code? Also, we should deprecate this key, but I'm can't see how to achieve that with JSON. In the future, the page bundle and all other dependent bundles should rely on standard http status codes instead of custom OK / KO codes

maarekj · 2017-06-13T16:35:13Z

Effectively it was a bc break in my code

Added support for editable association fields from type choice in Lis…

811dfb5

…tMapper

SonataCI added the review required label Apr 11, 2017

greg0ire added the minor label Apr 12, 2017

greg0ire reviewed Apr 12, 2017

View reviewed changes

greg0ire added Needs: Documentation Needs: Tests labels Apr 12, 2017

christingruber added 3 commits April 12, 2017 12:53

Fix store original value for editables from request before transforming

ea45a19

Fix http status handling for json responses on editables

9d4cc2b

Added documentation for editable association from type choice

d0abfe3

greg0ire requested changes Apr 12, 2017

View reviewed changes

greg0ire reviewed Apr 12, 2017

View reviewed changes

christingruber added 2 commits April 12, 2017 18:22

Update spelling for error messages

dd56081

Tiny changes for cs

daaeb14

greg0ire requested changes Apr 12, 2017

View reviewed changes

greg0ire approved these changes Apr 13, 2017

View reviewed changes

greg0ire removed the Needs: Documentation label Apr 13, 2017

christingruber added 2 commits April 13, 2017 12:18

Update spelling for error messages

7fce916

Updated existing tests for new json responses

712afaf

greg0ire removed the Needs: Tests label Apr 13, 2017

core23 approved these changes Apr 24, 2017

View reviewed changes

greg0ire merged commit d8493f1 into sonata-project:3.x Apr 24, 2017

OskarStark removed the review required label Jun 14, 2017

mvhirsch mentioned this pull request Jul 14, 2017

Page-Composer: removes custom status-code check (relying on Response-Status instead) sonata-project/SonataPageBundle#839

Merged

2 tasks

jordisala1991 mentioned this pull request Feb 4, 2018

[Proposal] Edit inline with Entities Reference in choice field type. #4188

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for editable association fields from type choice in Lis… #4449

Added support for editable association fields from type choice in Lis… #4449

christingruber commented Apr 11, 2017 •

edited by greg0ire

greg0ire Apr 12, 2017

greg0ire Apr 12, 2017

christingruber Apr 12, 2017 •

edited

greg0ire Apr 12, 2017

greg0ire left a comment •

edited

greg0ire Apr 12, 2017

greg0ire Apr 12, 2017

greg0ire commented Apr 12, 2017

greg0ire Apr 12, 2017

greg0ire Apr 12, 2017

christingruber Apr 13, 2017

greg0ire commented Apr 13, 2017 •

edited

greg0ire commented Apr 24, 2017

vworldat commented May 3, 2017

greg0ire commented May 3, 2017

vworldat commented May 3, 2017 •

edited

greg0ire commented May 3, 2017 •

edited

maarekj commented Jun 13, 2017

Added support for editable association fields from type choice in Lis… #4449

Added support for editable association fields from type choice in Lis… #4449

Conversation

christingruber commented Apr 11, 2017 • edited by greg0ire

Changelog

To do

Subject

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christingruber Apr 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Apr 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Apr 13, 2017 • edited

greg0ire commented Apr 24, 2017

vworldat commented May 3, 2017

greg0ire commented May 3, 2017

vworldat commented May 3, 2017 • edited

greg0ire commented May 3, 2017 • edited

maarekj commented Jun 13, 2017

christingruber commented Apr 11, 2017 •

edited by greg0ire

christingruber Apr 12, 2017 •

edited

greg0ire left a comment •

edited

greg0ire commented Apr 13, 2017 •

edited

vworldat commented May 3, 2017 •

edited

greg0ire commented May 3, 2017 •

edited