Problem/Motivation

Collect demo doesn't show the sample article on the front page.

Proposed resolution

In collect_demo.install add / before node/.

As a part of this issue, we should add a simple CollectDemoTest so we can detect bugs like this one. See MonitoringDemoTest.

Remaining tasks

User interface changes

API changes

Data model changes

Comments

mbovan’s picture

Issue summary: View changes
juanse254’s picture

Status: Active » Needs review
StatusFileSize
new3.32 KB

Should address the issue about front page not showing up but the test should be a different issue.

miro_dietiker’s picture

Status: Needs review » Needs work

This patch will not apply. Meanwhile i have pushed something.
Pull and merge first.

mbovan’s picture

+++ b/client/src/Plugin/QueueWorker/CollectClientQueueWorker.php
@@ -245,12 +245,12 @@ class CollectClientQueueWorker extends QueueWorkerBase implements ContainerFacto
-      $message = $exception->getMessage();
+      $message = '';
       // Get a response.
       if ($response = $exception->getResponse()) {
+        $message = $this->parseResponse($response, 'message') ?: '';
         // In case of server error, suspend further handling of the queue.
         if ($response->getStatusCode()[0] == '5') {
-          $message = $this->parseResponse($response, 'message') ?: $message;
           throw new SuspendQueueException($message, 0, $exception);
         }

Same as (1) in https://www.drupal.org/node/2472479#comment-10082650.

juanse254’s picture

Status: Needs work » Needs review
StatusFileSize
new2.38 KB
new731 bytes

Just rebased.

miro_dietiker’s picture

Status: Needs review » Needs work

I just wanted to commit, but this is a nice nitpick.

+++ b/demo/src/Tests/CollectDemoTest.php
@@ -0,0 +1,46 @@
+
+  /**
+   * {@inheritdoc}
+   */
+

Ah yeah, what?

juanse254’s picture

Status: Needs work » Needs review
StatusFileSize
new2.38 KB
new1.28 KB

Fixed this about the comment, also improved the url to be dynamic for the test.

mbovan’s picture

Status: Needs review » Needs work

This test is not actually tested (check test results). That's the reason why we get green tests.

  1. +++ b/demo/src/Tests/CollectDemoTest.php
    @@ -0,0 +1,40 @@
    + * @file
    + * Contains \Drupal\system\Tests\DrupalKernel\CollectDemoTest.
    + */
    

    Wrong path.

  2. +++ b/demo/src/Tests/CollectDemoTest.php
    @@ -0,0 +1,40 @@
    +
    +namespace Drupal\collect_demo\tests;
    +
    

    The namespace should be Drupal\collect_demo\Tests.

  3. +++ b/demo/src/Tests/CollectDemoTest.php
    @@ -0,0 +1,40 @@
    +    $this->assertText(t('collect:collectjson-definition/global/fields', array('@host' => $host)));
    

    The host parameter is not present here.

  4. +++ b/demo/src/Tests/CollectDemoTest.php
    @@ -0,0 +1,40 @@
    +    $this->clickLink(t('view'));
    

    View with capital "V".

  5. +++ b/demo/src/Tests/CollectDemoTest.php
    @@ -0,0 +1,40 @@
    +    $this->assertResponse('200');
    

    Http code should be passed as an integer.

  6. +++ b/demo/src/Tests/CollectDemoTest.php
    @@ -0,0 +1,40 @@
    +    $this->assertText(t('collect:collectjson/@host/entity/user"', array('@host' => $host)));
    

    Extra double quote character in 'collect:collectjson/@host/entity/user"'

  7. +++ b/demo/src/Tests/CollectDemoTest.php
    @@ -0,0 +1,40 @@
    +    $this->clickLink(t('view'));
    +    $this->drupalGet('/');
    

    We don't really test anything here.

  8. @@ -0,0 +1,40 @@
    diff --git a/src/Tests/CollectWebTest.php b/src/Tests/CollectWebTest.php
    index 9e4ad53..23903cc 100644
    
    --- a/src/Tests/CollectWebTest.php
    +++ b/src/Tests/CollectWebTest.php
    @@ -83,7 +83,6 @@ class CollectWebTest extends WebTestBase {
           'date' => (new DrupalDateTime('2014-12-16 09:40'))->getTimestamp(),
         ));
         $container->save();
    -
         $this->drupalGet('admin/content/collect');
         $this->assertResponse(403);
    

    Unrelated change?

miro_dietiker’s picture

We don't really test anything here.

We do. It would report / throw an exception/error, if it fails. Also the response code is checked below. Which would fail in this issue with a test-only patch.

juanse254’s picture

Status: Needs work » Needs review
StatusFileSize
new2.33 KB
new1.51 KB

Fixed with suggestions on comment #8.

Status: Needs review » Needs work

The last submitted patch, 10: Demo_article_not_displayed-2519966-10.patch, failed testing.

mbovan’s picture

Created an issue for broken ProcessingWebTest #2527148: Adding processors to the model is broken

The one of the reasons why CollectDemoTest failed is because CRM Core module is not on d.o (collect_demo has a dependency on it), so we cannot run the test properly...

I suggest to upload 2 patches, one with the leading change so we can commit that and the other one that will contain only CollectDemoTest (Named something like demo_article_not_displayed-2519966-12-DO-NOT-TEST.patch) so we can add the tests once CRM Core is on d.o.

P.S. We can still do "8" from the comment #8.

miro_dietiker’s picture

  1. +++ b/demo/src/Tests/CollectDemoTest.php
    @@ -0,0 +1,38 @@
    +    $this->assertText(t('collect:collectjson/@host/entity/node/article', array('@host' => $host)));
    +    $this->assertText(t('collect:collectjson-definition/global/fields'));
    +    $this->assertText(t('collect:collectjson/@host/entity/user', array('@host' => $host)));
    +    $this->assertText(t('collect:collectjson-definition/global/fields'));
    

    Those are not translatable! It's just a placeholder replacement.

  2. +++ b/src/Tests/CollectWebTest.php
    @@ -83,7 +83,6 @@ class CollectWebTest extends WebTestBase {
    -
    

    And yeah still unrelated change.

mbovan’s picture

As #2527148: Adding processors to the model is broken is committed, Collect is green again. We can continue to work on this issue. Retesting...

The last submitted patch, 10: Demo_article_not_displayed-2519966-10.patch, failed testing.

juanse254’s picture

Status: Needs work » Needs review
StatusFileSize
new572 bytes
new1.34 KB

Status: Needs review » Needs work

The last submitted patch, 17: Demo_article_not_displayed-2519966-17_TEST_ONLY.patch, failed testing.

mbovan’s picture

Status: Needs work » Reviewed & tested by the community

Cool!

We can commit the patch, and continue to work on fixing CollectDemoTest locally.

Also, we can open an (postponed) issue (similar to #2430111: Re-add tests when CRM Core is on drupal.org) and upload fixed CollectDemoTest there.

miro_dietiker’s picture

Status: Reviewed & tested by the community » Needs work

Not really. The fixing patch does not contain the test!

miro_dietiker’s picture

Status: Needs work » Needs review
StatusFileSize
new1.9 KB

Providing combined patch for testbot.

Status: Needs review » Needs work

The last submitted patch, 21: collect_2519966_demo_21.patch, failed testing.

miro_dietiker’s picture

OK... The tests also don't pass locally because of some Schema config error. Let's fix this first.

miro_dietiker’s picture

Note: Editing the core site information even throws an exception when data is faulty... See admin/config/system/site-information

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 21: collect_2519966_demo_21.patch, failed testing.

miro_dietiker’s picture

Status: Needs work » Postponed
Issue tags: +crm core

Committed the fix. Tests can't work as long as crm core is not on d.o.
Postponing the demo test for crm core.

  • miro_dietiker committed 84ac794 on 8.x-1.x
    Issue #2519966 by juanse254, miro_dietiker: Demo article is not...
mbovan’s picture

Status: Postponed » Fixed

Test is moved into #2430111: Re-add tests when CRM Core is on drupal.org so we can mark this issue as fixed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.