Aggregator module UI text cleanup

Why did you remove some descriptions?

Need to alter .test files where these sentences are being used on assertions (see failed tests)

This needs to be re-rolled, regardless of the need to fix tests and such. yoroy, not sure if you're not working on this anymore, but if not, just unassign. This is great for novice.

Rerolled. Applies cleanly at commit bb3270d.

The failing tests are probably due to assertions on the UI text that was changed. Should be really easy to fix.

I'll work on this tonight. I was uncertain of when to assign an issue to myself, are there any docs regarding the ettiquette of assigning an issue?

@kid_icarus, if it's unassigned, it's ok to assign it to you when you start working on it and unassign when you're no longer doing so, just so to prevent 2 or more pople doing the same work. Go on! =)

I Modified aggregator.test assertions to account for the texual changes in the original patch. If the patch passes I think those texual changes could use some review.

@franz, thanks for the tips!

+++ b/core/modules/aggregator/aggregator.admin.inc
@@ -200,7 +198,7 @@ function aggregator_form_feed_submit($form, &$form_state) {
+    watchdog('aggregator', 'Feed %feed added.', array('%feed' => $form_state['values']['title']), LOG_NOTICE, l(t('view'), 'admin/config/services/aggregator'));

The correct is WATCHDOG_NOTICE, as you can check in watchdog_severity_levels()

+++ b/core/modules/aggregator/aggregator.admin.inc
@@ -254,26 +252,26 @@ function aggregator_form_opml($form, &$form_state) {
+    '#description' => t('Upload an OPML file with a list of feeds to  import.'),

extra space.

+++ b/core/modules/aggregator/aggregator.admin.inc
@@ -254,26 +252,26 @@ function aggregator_form_opml($form, &$form_state) {
+    '#description' => t('This file will be downloaded and processed only once on submission of the form.'),

How about "... only once on form submission" ?

1 days to next Drupal core point release.

The WATCHDOG_NOTICE issue shows up twice in the patch, Dreditor didn't save the second comment on it.

I've been told that screenshots will help contextualize those UI text changes.

Addressed #14. Attached are some screenshots as requested in #17 :)

Whoops, forgot that second instance of LOG_NOTICE. Patched.

Patch reviewed, looks good.

#20: aggregator-uitext-20.patch queued for re-testing.

Re-rolled to account for #1585944: Convert aggregator tests to PSR-0

Applies cleanly at 9532b4b

#24: aggregator-uitext-24.patch queued for re-testing.

#24: aggregator-uitext-24.patch queued for re-testing.

Patch rerolled against latest head.

Re-rolled. Tests were changed and t('') was dropped on messages, so I updated the patch to reflect this as well.

Cross-referencing #398578: Remove 'news' language from aggregator.

Cross-referencing #1906692: String cleanup: make strings consistent in DefaultFetcher::fetch.

#30: aggregator-uitext.patch queued for re-testing.

Attached is a reroll of the patch in comment #18. It also is work done within http://core.drupalofficehours.org/node/1563/edit

It applied cleanly, drupal didn't blow up, and it needs someone who knows the aggregator module well to test it for functionality.

I'm new to contributing to core so please be gentle :)

So I looked at the simpletest that failed, and someone simply changed the help text, so the assertText fails.

The test is looking for "A single OPML document may contain a collection of many feeds.', 'Found OPML help text"

But now someone has changed the help text to be: "A single OPML document may contain a collection of many feeds"

Here is the simpletest that fails for your convenience: http://api.drupal.org/api/drupal/core%21modules%21aggregator%21lib%21Dru...

So, at this point I think my reroll of the patch #18 was fine but whomever wrote patch #18 changed the help text such that this simpletest failed. Should the simpletest change or should I just change the text and submit a new patch. Perhaps the text was changed for good reason?

Yes says in http://drupal.org/node/1209674#comment-5785106 above that tests are failing just because of assertText and the authors of the patch think their text changes were for good reason. I'm not sure what the procedure is here .... should I continue in this same ticket and change the assertText in the simpletest so this patch above will pass?

First of all, thanks barnettech for rerolling this. I'm sure it was quite the task! A couple of issues though

1) The patch brings back a bunch of functions that are no longer in core. In general, a good sanity check when rerolling patches is to compare the diff to the one you rerolled. In this case, being a text clean-up, if you see any changes other than UI text in your patch, you should probably start over.

2) Always reroll the latest patch unless otherwise stated in a comment in the issue, or on the COH site. In this case, I went with the patch in comment #30.

Again thanks! Please don't let my reroll here discourage you, just trying to move the issue along!

If mine has the same test failures yours did, I highly encourage you to fix the assert text in those tests so we can ge this committed!

#38: drupal-aggregrator_module_ui_cleanup-1209674-38.patch queued for re-testing.

Could not replicate any of these locally running the failed tests individually. Sending back to testbot to see if it was just a fluke.

I was told by kbasarab to reroll the patch in #18. My reroll only failed in simpletests for text changes as I said. I'm surprised if there are functions that are no longer in core that the simpletests didn't pick up any other problems.

xjm explained that some of the functions may have been older and deprecated but still work, but deferred to you that #30 should have been rerolled. I'm not sure why kbasarab said to reroll #18, I only ask because I hope we are correct in re-rolling #30 now.

Rerolling patch #38 against current head

I checked the patch it looks good compared to the existing code in the module. I do however have some additional comments that i would like your opinion on.

Things that are wrong

1. There was no title in hook_menu() for the 'admin/config/services/aggregator' path. Perhaps hook_menu() will die in the next few months but currently this causes an UI issue.
2. The description text points to a 'latest items' block which does not exist. The block name is either Aggregator feed or Aggregator category

Things that could be done differently

1. The links for supported formats (RSS, RDF, ATOM) in the description text point to fairly different sites. Wouldn't it be better to link to a single more "official" site like Wikipedia or w3.org?
2. The text "Requires a correctly configured cron maintenance task" does not require the word correctly. It seems pretty obvious to me that it shouldn't be configured wrong.

Please let me know if you agree with the things mentioned above and i will gladly provide another patch.

Regarding Things that are wrong:

• The Title for admin/config/services/aggregator is best corrected in a different issue. IMHO it is out of scope of this issue.
• The reference to "latest items" should be corrected (in this issue).

Regarding Things that could be done differently:

• Feel free to suggest/use different links for supported formats (RSS, RDF, ATOM) .
• The text "Requires a correctly configured cron ..." text is in this issue replaced by "A working cron maintenance task ...". I think this fixes your point.

44: drupal8.user-interface-text.1209674-44.patch queued for re-testing.

Checking against current HEAD.

Working on rerolling patch against current head.

Rerolled patch to include changes from HEAD and better links to wikipedia.

Rerolled patch to reflect recent changes in aggregator module.

This looks great apratt. I was able to apply this to a fresh install of D8 and see the changes outlined in the patch on the aggregator screens.

The language is indeed more approachable by removing the should and must wording. I like the proposed links pointing towards Wikipedia as well.

+1 for RTBC

53: drupal8.user-interface-text.1209674-53.patch queued for re-testing.

The patch undoes some of the improvements from #1977608: Update hook_help Aggregator module.

-      $output .= '<dd>' . t('A correctly configured <a href="!cron">cron maintenance task</a> is required to update feeds automatically.', array('!cron' => \Drupal::url('system.cron_settings'))) . '</dd>';
+      $output .= '<dd>' . t('A working <a href="@cron">cron maintenance task</a> is required to update feeds automatically.', array('@cron' => 'http://drupal.org/cron')) . '</dd>';

How about:

+      $output .= '<dd>' . t('A working <a href="!cron">cron maintenance task</a> is required to update feeds automatically.', array('!cron' => \Drupal::url('system.cron_settings'))) . '</dd>';

Good catch I'll reroll the patch and retest it.

Angus

Made the suggested changes to patch.

Bad edit - this time I have the patch working before I upload it. That's what I get for doing it at 1:00am.

60: drupal8.user-interface-text.1209674-60.patch queued for re-testing.

Regarding the changes to hook_help(), there are several problems -- it looks like maybe there was a blind reroll of this patch without looking at what changes had been made that conflicted:

a) There was a reason that the URLs were put into the translated help text in these lines:

      $output = '<p>' . t('Thousands of sites (particularly news sites and blogs) publish their latest headlines and posts in feeds, using a number of standardized XML-based formats. Formats supported by the aggregator include <a href="http://cyber.law.harvard.edu/rss/">RSS</a>, <a href="http://www.w3.org/RDF/">RDF</a>, and <a href="http://www.atomenabled.org">Atom</a>.') . '</p>';
-      $output .= '<p>' . t('Current feeds are listed below, and <a href="!addfeed">new feeds may be added</a>. At the <a href="!block">blocks administration page</a> you can enable for each feed the block <em>Aggregator feed</a> that contains the <em>latest items</em> .', array('!addfeed' => \Drupal::url('aggregator.feed_add'), '!block' => \Drupal::url('block.admin_display'))) . '</p>';
+      $output = '<p>' . t('Many sites publish their headlines and posts in feeds, using a number of standardized XML-based formats. The aggregator supports <a href="@rss">RSS</a>, <a href="@rdf">RDF</a>, and <a href="@atom">Atom</a>.', array('@rss' => 'http://en.wikipedia.org/wiki/Rss', '@rdf' => 'http://en.wikipedia.org/wiki/Resource_Description_Framework', '@atom' => 'http://en.wikipedia.org/wiki/Atom_%28standard%29')) . '</p>';
+      $output .= '<p>' . t('Current feeds are listed below, and <a href="@addfeed">new feeds may be added</a>. For each feed, the <em>latest items</em> block may be enabled at the <a href="@block">blocks administration page</a>.', array('@addfeed' => url('admin/config/services/aggregator/add/feed'), '@block' => url('admin/structure/block'))) . '</p>';

The reason being that the RSS, RDF, and Atom links point specifically to English reference sites. A translator might want to provide a different URL that would work for their language.

b) I think the reason it says "include" in the current help is that other modules can add support for other formats.

c) You don't want to use @url for a link, but !url.

d) Why make this change?

-      $output .= '<dd>' . t('A correctly configured <a href="!cron">cron maintenance task</a> is required to update feeds automatically.', array('!cron' => \Drupal::url('system.cron_settings'))) . '</dd>';
+      $output .= '<dd>' . t('A working <a href="!cron">cron maintenance task</a> is required to update feeds automatically.', array('!cron' => \Drupal::url('system.cron_settings'))) . '</dd>';       $output .= '</dl>';

e) This is also wrong -- the "acronym" tag is deprecated:

-      return '<p>' . t('<abbr title="Outline Processor Markup Language">OPML</abbr> is an XML format used to exchange multiple feeds between aggregators. A single OPML document may contain a collection of many feeds. Drupal can parse such a file and import all feeds at once, saving you the effort of adding them manually. You may either upload a local file from your computer or enter a URL where Drupal can download it.') . '</p>';
+      return '<p>' . t('<acronym title="Outline Processor Markup Language">OPML</acronym> is an XML format for exchanging feeds between aggregators. A single OPML document may contain many feeds. Aggregator uses this file to import all feeds at once. Upload a file from your computer or enter a URL where the OPML file can be downloaded.') . '</p>';

The other UI text changes look fine. Thanks!

Regarding:

d) comments #45, #46, and #57 seemed to have come to the conclusion that it must be correctly configured or many things will not work, thus the change.

e) That is a good catch. The tag has been deprecated. I missed that in an earlier review.

Thanks

This patch does not apply to head any more. I will do a reroll first and then add the changes per comments in #1209674-63: Aggregator module UI text cleanup.

Working on this now.

Here is the rerolled patch.

I have tested the reroll in simplytest.me.

The "new feeds may be added" link is not correct. It is pointing to admin/config/services/aggregator/add/feed and appears that it should be pointing to aggregator/sources/add. I will change the patch to point to the new path \Drupal::url('aggregator.feed_add').

I'm not clear on the discussion about the "latest items" issue. Currently there is no link on that text, but there is a link to "go to the block administration page", which does link correctly to admin/structure/block, although it uses the incorrect syntax for the URL; I will fix the syntax. So, I don't know if this fixes the problem or not.

I will correct @jhogdon's a, c, and e points.

I don't know what to do about b (use of the word "include" for what formats aggregator supports). I think there would need to be more discussion about how to do the rewording. Therefore, I'm leaving that change as is (was) in the patch.

I am also leaving the change that @jhodgon referred to in point d as is (was) in the patch.

Please don't change things on this issue. I am the middle of working on it. First I completed the reroll and now I'm completing the interdiff.

Here is the new patch and the interdiff. Don't set it to "Need review" yet. I want to check it on simplytest.me first.

D'oh! There is one typo in "widipedia". Let me fix that now.

Here is the latest patch and interdiff. Let me check it quickly again in simplytest.me.

Okay, looks good in simplytest.me. Time for the testbot to have a look!

Hm... Looking at the test:

+    $this->assertRaw(t('Upload a file or enter a URL.'), 'Error if both fields are filled.');

This test seems to be saying that the error message is being shown either if neither a file nor a URL is entered, or if both are entered. Maybe the error message should say "... , but not both"?

-      $this->setFormError('remote', $form_state, $this->t('You must <em>either</em> upload a file or enter a URL.'));
+      $this->setFormError('remote', $form_state, $this->t('Upload a file or enter a URL.'));

It looks like the previous message emphasized the "either", and this was lost when the patch was made?

Please review updated patch with fixes in #75.

Um... It looks like you updated the message that the module produces one way, and the tests in two different ways, so the tests are failing.

I believe the text in test cases should match with the text in module file (core/modules/aggregator/src/Form/OpmlFeedAdd.php).

That's right! If you change the UI text, the tests that check for that UI test need to be changed in the same way. :)

Anyway, this looks good to me. Any other opinions?

Thanks for bringing this to fruition. It looks good to me.

Good catch yoroy!

Removed the extra

Thanks!

Random unrelated test failure there.

84: drupal8.user-interface-text.1209674-84.patch queued for re-testing.

Thanks everyone again! Committed to 8.x.