Aggregator feed overview misleading text

1. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/FeedAdminDisplayTest.php
   @@ -0,0 +1,56 @@
   + * Tests feed page logic to display synchronization information on admin pages.
   

   i would suggest something more generic, eg "Tests display of admin pages"

2. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/FeedAdminDisplayTest.php
   @@ -0,0 +1,56 @@
   +class FeedAdminDisplayTest extends AggregatorTestBase {
   +  public static function getInfo() {
   

   the function declaration needs a newline before

3. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/FeedAdminDisplayTest.php
   @@ -0,0 +1,56 @@
   +  }
   +
   +
   +  /**
   

   too many newlines here, one should be removed

4. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/FeedAdminDisplayTest.php
   @@ -0,0 +1,56 @@
   +    // Enable test plugins.
   +    $this->enableTestPlugins();
   

   Why this test needs the test plugins?

5. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/FeedAdminDisplayTest.php
   @@ -0,0 +1,56 @@
   +    $this->assertResponse(200, 'Feed source exists.');
   

   hm? Feed source?

6. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/FeedAdminDisplayTest.php
   @@ -0,0 +1,56 @@
   +  }
   +}
   

   Needs a newline in between of }

Working on this

Updated this patch to latest 8.0.x

- Added extra test to ensure both scheduled and non scheduled feeds show correct update/next field.
- Followed suggestions from #2

1. +++ b/core/modules/aggregator/src/Tests/FeedAdminDisplayTest.php
   @@ -0,0 +1,57 @@
   + * Definition of Drupal\aggregator\Tests\FeedAdminDisplayTest.
   

   Should be Contains \Drupal\..

2. +++ b/core/modules/aggregator/src/Tests/FeedAdminDisplayTest.php
   @@ -0,0 +1,57 @@
   +    $this->assertText(t('never'), 'The scheduled feed has not been updated yet.  Last update shows "never".');
   +    $this->assertText(t('immediately'), 'The scheduled feed has not been updated yet. Next update shows "immediately".');
   

   how both assertions can be true, if we only have one feed created?

   Also i think we can safely remove t() calls from the assertions in this test

Thx for the review ParisLiakos

Changed file description as stated in 5.1, and removed t() calls.

For 5.2, the first check is for the "last update" field, the second is for the "next update field", see attached screenshot.

oh ok, i see thanks a lot for the screenshot! this is good to go now

Mainly nits.

1. +++ b/core/modules/aggregator/src/Controller/AggregatorController.php
   @@ -126,7 +126,16 @@ public function adminOverview() {
   +      if(!$last_checked && $refresh_rate) {
   

   Missing space.

2. +++ b/core/modules/aggregator/src/Tests/FeedAdminDisplayTest.php
   @@ -0,0 +1,57 @@
   +    // The scheduled feed shows that it has not been updated yet and is scheduled.
   

   This and most other comments in the test over 80 chars.

3. +++ b/core/modules/aggregator/src/Tests/FeedAdminDisplayTest.php
   @@ -0,0 +1,57 @@
   +    $this->assertNoText('never', 'The scheduled feed has been updated. Last updated changed.');
   

   Is there a positive assertion we can do as well?

Changed code following suggestions in #8

Added extra tests for positive assertions.

would be nice to post interdiffs next time ;)
thanks!

correcting to official amsterdam tag

Removing autotag needs work from testbot, patch is green and tested

reroll

+++ b/core/modules/aggregator/src/Controller/AggregatorController.php
@@ -126,7 +126,16 @@ public function adminOverview() {
+        $next_update = $this->t('immediately');

But it is not really "immediately" is it? It is the next cron run. Or at best "imminently"

"imminently" sounds better, yes

Rerolled the patch as per the latest D8 release.

Removing need reroll tag..

Some nitpicks.

1. +++ b/core/modules/aggregator/src/Tests/FeedAdminDisplayTest.php
   @@ -0,0 +1,71 @@
   +
   +
   

   Let's get rid of all these double empty lines in the test.

2. +++ b/core/modules/aggregator/src/Tests/FeedAdminDisplayTest.php
   @@ -0,0 +1,71 @@
   +    // Delete scheduled feed
   

   Inline comments should end with a "."

3. +++ b/core/modules/aggregator/src/Tests/FeedAdminDisplayTest.php
   @@ -0,0 +1,71 @@
   +    // Create non-scheduled feed
   

   Same as above

4. +++ b/core/modules/aggregator/src/Tests/FeedAdminDisplayTest.php
   @@ -0,0 +1,71 @@
   +    // After the feed update, we still need to see "never" as next update
   +    // label.  Last update will show an interval.
   

   "label." still fits on the first line here.

Looks good, will RTBC when test becomes green.

Setting back to RTBC as per #29

I think, that was test-bot glitch

Yes, this was a testbot glitch.

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 12b8a2f and pushed to 8.0.x. Thanks!