@shaal raised this reviewing #3133017: Add rector coverage info to errors that we should group errors by type. This would even allow us to have instructions / descriptions for what each type means right at the list, rather than somewhere else. It would allow us to funnel people much easier into using rector as well. If we remove the repeating type column that would also allow us to have a more readable output. So let's see what we can do for that.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy created an issue. See original summary.

Gábor Hojtsy’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
9.54 KB
246.79 KB
166.42 KB

Here is a start. I don't like the pieces of bland text at the top, they are not very well structured. Probably need a definition list type of setup or somesuch. I am not a designer, just trying to do my best :D suggestions welcome.

BEFORE:

AFTER:

Note that we are funeling people into being rector users at the automatic fix category and rector contributors at the manual fix category ;)

Status: Needs review » Needs work

The last submitted patch, 2: 3134167.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Kristen Pol’s picture

I'll take a look even though it's not done yet.

Gábor Hojtsy’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
9.56 KB
1.18 KB
168.24 KB

Wrong category for uncategorized :D Haha.

We may want to change the color / image of the "Ignore" category to not be similar to the category above it :D Also an exclamation mark in a triangle for stuff to ignore does not match well for what we mean there.

Status: Needs review » Needs work

The last submitted patch, 5: 3134167-3.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Kristen Pol’s picture

I quickly scanned the code and only noticed some nitpicks. As for the UI, I didn't test it because STM is down right now but, looking at the images you provided, 1) this is great (love the grouping :), 2) some of the leading info could be tweaked a little but these are nitpicks as well (see below).

Formatting:

  1. +++ b/css/upgrade_status.admin.theme.css
    @@ -45,6 +45,7 @@
    +.upgrade-status-error-list tr > td.status-info, ¶
    

    Nitpick: Extra space at end of line.

  2. +++ b/src/ScanResultFormatter.php
    @@ -100,9 +100,14 @@ class ScanResultFormatter {
    +            'No deprecation scanning data available. <a href="@url">Go to upgrade status form</a>.', ¶
    

    Nitpick: Capitalize "upgrade status"?

    Nitpick: Extra space at end of line.

  3. +++ b/src/ScanResultFormatter.php
    @@ -267,6 +240,64 @@ class ScanResultFormatter {
    +        $this->t("Based on the Drupal deprecation version number of these, fixing them will likely make them incompatible with your current Drupal version."),
    

    Nitpick: Double quotes could be single quotes.

Wording:

  1. +++ b/src/ScanResultFormatter.php
    @@ -267,6 +240,64 @@ class ScanResultFormatter {
    +        $this->t('Avoid some manual work by using <a href="@drupal-rector">drupal-rector to fix issues automatically</a> or <a href="@upgrade-rector">Upgrade Rector</a> to generate patches.', ['@drupal-rector' => 'https://www.drupal.org/project/rector', '@upgrade-rector' => 'https://www.drupal.org/project/upgrade_rector']),
    

    Nitpick: Based on the structure, it's unclear if "to generate patches" is meant to be for "Upgrade Rector" only or for both "drupal-rector" and "Update Rector" since "drupal-rector" already has a "to fix issues automatically" in the link. If it's only for "Update Rector", I would move "to generate patches" within the link.

  2. +++ b/src/ScanResultFormatter.php
    @@ -267,6 +240,64 @@ class ScanResultFormatter {
    +        $this->t('It does not seem like these are covered by automation yet. <a href="@drupal-rector">Contribute to drupal-rector to provide one</a>. Fix manually in the meantime.', ['@drupal-rector' => 'https://www.drupal.org/project/rector']),
    

    Nitpick: "to provide one" seems unclear. How about "to provide coverage"?

  3. +++ b/src/ScanResultFormatter.php
    @@ -267,6 +240,64 @@ class ScanResultFormatter {
    +        $this->t('Error without Drupal source version numbers including parse errors and use of APIs from dependencies.'),
    

    Nitpick: Change "Error" to "Errors" or "Error(s)" or restructure to use "These" like in some other text.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
9.93 KB
3.56 KB

Fixed all of those, thanks @Kristen Pol!

Also fixed the explanation for the "fix later" category since its actually different for contrib projects vs. custom projects.

Status: Needs review » Needs work

The last submitted patch, 8: 3134167-8.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
234.83 KB
16.78 KB
8.1 KB

Then went on to add the capability to alter the result display, so upgrade_rector can change it. This results in this feature:

https://www.drupal.org/files/issues/2020-05-07/Screen%20Recording%202020...

Still need to fix tests / the HTML export but otherwise should be fine.

Status: Needs review » Needs work

The last submitted patch, 10: 3134167-10.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
17.66 KB
901 bytes

Fix export which should make those tests pass too (they pass locally) :) Fixed it visually too.

Gábor Hojtsy’s picture

Committed a upgrade_rector update to integrate with this expecting this landing :D https://git.drupalcode.org/project/upgrade_rector/-/commit/e06d65cd38034...

  • Gábor Hojtsy committed f151735 on 8.x-2.x
    Issue #3134167 by Gábor Hojtsy, Kristen Pol: Group errors find by type (...
Gábor Hojtsy’s picture

Status: Needs review » Fixed

Landed this. There is probably still more optimisations that could happen. But we can do them in future updates.

Kristen Pol’s picture

Woot! Thanks! Took a quick look and...

+++ b/src/ScanResultFormatter.php
@@ -267,6 +256,75 @@ class ScanResultFormatter {
+        // Issues to fix later need differen guidance based on whether they

Typo to fix later: "differen guidance" => "different guidance".

  • Gábor Hojtsy committed 39342ec on 8.x-2.x
    Issue #3134167 followup by Gábor Hojtsy, Kristen Pol: Fix typo in...
Gábor Hojtsy’s picture

Thanks, fixed that and also added a unique icon to the "fix later" category. Also no coloring there.

Kristen Pol’s picture

Looks great!

Status: Fixed » Closed (fixed)

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