Api product association #25

peterserfozo · 2018-06-06T23:21:46Z

No description provided.

mxr576 · 2018-06-11T06:32:56Z

src/Entity/Form/DeveloperAppCreateForm.php

@@ -73,6 +73,9 @@ public static function create(ContainerInterface $container) {

  /**
   * {@inheritdoc}
+   *
+   * @throws \Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException


I am little bit sceptical whether we should document all possible exceptions that Phpstorm offers for us. If we know for sure that an exception should not be thrown in normal circumstances (confidence lvl 99%) then we should not document it, because it could be misleading.
Ex.: \Drupal\Component\Plugin\Exception\PluginNotFoundException should never be thrown because "api_product" entity always exists in our module.

mxr576 · 2018-06-11T06:34:35Z

src/Entity/Form/DeveloperAppCreateForm.php

@@ -208,38 +209,38 @@ protected function actions(array $form, FormStateInterface $form_state) {

  /**
   * {@inheritdoc}
+   *
+   * @throws \Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException


Same as above.

mxr576 · 2018-06-11T06:36:24Z

src/Form/AppSettingsForm.php

-        ':link' => Url::fromRoute('apigee_edge.settings')->toString(),
-      ]);
-    }
+    // It's necessary to add a wrapper, because if the ID is added to the


Unnecessary comma.

mxr576 · 2018-06-11T06:36:45Z

src/Form/AppSettingsForm.php

-    }
+    // It's necessary to add a wrapper, because if the ID is added to the
+    // checkboxes form element, then that will not be properly rendered
+    // (the label is duplicated).


s/is/gets/g

src/Form/AppSettingsForm.php

-      ]);
-    }
+    // It's necessary to add a wrapper, because if the ID is added to the
+    // checkboxes form element, then that will not be properly rendered


mxr576 · 2018-06-11T06:38:18Z

src/Form/AppSettingsForm.php


-    $form['api_product']['default_api_product_multiple'] = [
+    $form['api_product']['default_api_product_multiple_container']['default_api_product_multiple'] = [
      '#type' => 'checkboxes',


You could have tried to use multiple checkbox form element if "checkboxes" that problematic :)

But I should create custom validation for the multiple checkbox form element approach (must choose at least one product if user_select is unchecked).

mxr576 · 2018-06-11T06:41:28Z

src/Form/AppSettingsForm.php

+   * @see https://www.drupal.org/project/drupal/issues/2855139
+   */
+  public function apiProductListCallback(array &$form, FormStateInterface $form_state) : array {
+    return $form['api_product']['default_api_product_multiple_container'];


I prefer if AJAX commands gets returned, like ReplaceCommand, instead of render elements directly.

https://api.drupal.org/api/drupal/core%21core.api.php/group/ajax/8.5.x

…nto api-product-association

…gee-edge-drupal into api-product-association

…nto api-product-association

mxr576 · 2018-06-14T05:14:38Z

docker-compose.yml

@@ -58,5 +58,11 @@ services:
    depends_on:
      - php



This line is missing from the definition of the PHP container: https://github.com/mxr576/my-webdev-docker-setup/blob/master/docker-compose.yml#L52

mxr576 · 2018-06-14T05:19:21Z

tests/src/Functional/ApigeeEdgeTestTrait.php

+   * @throws \ReflectionException
+   *   If the class or object do not exist.
+   */
+  public static function getInvisibleProperty($object, $property_name) {


Do you still need this?
If not now then in the next sprint we can probably replace it with \PHPUnit\Framework\Assert::readAttribute.

I agree. I will improve the existing tests in the next sprint.

mxr576 · 2018-06-14T05:21:03Z

tests/src/FunctionalJavascript/AppSettingsFormTest.php

+ * App settings form tests.
+ *
+ * @group apigee_edge
+ * @group apigee_edge_developer_app


Maybe introducing a apigee_edge_javascript group would not be a bad idea for JS tests.

This is the reason why this failed: https://travis-ci.org/peterserfozo/apigee-edge-drupal/jobs/391985870#L2311

which is also the reason of the failing tests. We are not leveraging MINK_DRIVER_CLASS anymore so we do not need this patch. (The patch needs an update to fix regression bugs that it caused.)

googlebot · 2018-06-15T07:16:11Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

tests.

…ciation # Conflicts: # src/Form/AppSettingsForm.php # tests/src/Functional/ApigeeEdgeFunctionalTestBase.php

Péter Serfőző added 3 commits June 7, 2018 01:17

Removed associate_apps from config.

4971454

Developer app settings AJAX form.

0329dc2

Fixed developer app create and edit forms.

156638a

peterserfozo requested review from tamasd and mxr576 June 6, 2018 23:21

Merge branch '8.x-1.x' into api-product-association

f9a379a

tamasd approved these changes Jun 8, 2018

View reviewed changes

mxr576 suggested changes Jun 11, 2018

View reviewed changes

Péter Serfőző added 13 commits June 11, 2018 16:37

Merge branch '8.x-1.x' of github.com:apigee/apigee-devportal-drupal i…

1f7a88b

…nto api-product-association

Fixed comments, AJAX callback.

5252f9a

Fixed Developer App UI test, refactored base class of functional tests.

80f3798

Merge branch 'api-product-association' of github.com:peterserfozo/api…

4aa39b0

…gee-edge-drupal into api-product-association

Refactored functional tests.

7ff8867

FunctionalJavascript base class and app settings AJAX form test.

93421fd

Merge branch '8.x-1.x' of github.com:apigee/apigee-devportal-drupal i…

35cc765

…nto api-product-association

Removed unnecessary tearDown().

9a8cd6f

Revert changes.

5c2ec3c

Drupal core patch for JS tests.

3a3ecad

Fixed CS.

e4efcec

Added Chromedriver to docker-compose for running JS tests.

33c5f3f

Fixed requirements test.

2126326

mxr576 suggested changes Jun 14, 2018

View reviewed changes

Péter Serfőző and others added 5 commits June 14, 2018 08:03

Added javascript test group.

47001bf

Added mink driver arguments to docker-compose.

ecfdaf4

Increased AJAX timeout.

7da1e23

Updated php, webserver versions in docker-compose.

92dfce6

Removing unnecessary patch

75dc2ee

which is also the reason of the failing tests. We are not leveraging MINK_DRIVER_CLASS anymore so we do not need this patch. (The patch needs an update to fix regression bugs that it caused.)

mxr576 and others added 2 commits June 15, 2018 09:17

Restoring default timeout values.

25f996b

Fixing failing Javascript tests caused by a permission issue.

5187470

mxr576 added 2 commits June 18, 2018 12:03

Upped PHP image version in Dockerfile too.

376d43a

Added a more advanced utility function for creating a screenshot in JS

01e6746

tests.

mxr576 force-pushed the api-product-association branch from 4a04148 to 01e6746 Compare June 18, 2018 10:41

mxr576 added 2 commits June 18, 2018 16:46

Merge remote-tracking branch 'upstream/8.x-1.x' into api-product-asso…

eaa9a67

…ciation # Conflicts: # src/Form/AppSettingsForm.php # tests/src/Functional/ApigeeEdgeFunctionalTestBase.php

Restored unintentionally deleted method.

6a8989d

mxr576 approved these changes Jun 18, 2018

View reviewed changes

mxr576 merged commit 555501c into apigee:8.x-1.x Jun 18, 2018

arunz6161 mentioned this pull request Nov 15, 2019

[Hybrid] Unable to create app on hybrid portal #296

Closed

ravindrasingh22 mentioned this pull request Feb 27, 2020

In one api fails, all other edge functionality gets blocked #339

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Api product association #25

Api product association #25

peterserfozo commented Jun 6, 2018

mxr576 Jun 11, 2018

mxr576 Jun 11, 2018

mxr576 Jun 11, 2018

mxr576 Jun 11, 2018

This comment was marked as resolved.

mxr576 Jun 11, 2018

peterserfozo Jun 13, 2018

mxr576 Jun 11, 2018

mxr576 Jun 14, 2018

peterserfozo Jun 14, 2018

mxr576 Jun 14, 2018

peterserfozo Jun 14, 2018

mxr576 Jun 14, 2018

mxr576 Jun 14, 2018

googlebot commented Jun 15, 2018

Api product association #25

Api product association #25

Conversation

peterserfozo commented Jun 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Jun 15, 2018