Description

The OG Linkchecker module allows designated OG roles to view group-specific Linkchecker reports so that group owners (for example) can find and fix broken links in their own groups.

For users with the "Access group broken links report" OG permission, a menu tab for a group Linkchecker report is added to the main group entity. This report behaves exactly like the administrative broken links report, but only shows links which appear in that group.

Users with the Linkchecker "Access broken links report" permission automatically gain access both to each group's broken links report and to the "OG Linkchecker groups summary" report, which provides the following information:

* The content type of each group as well as a link to the group broken links report
* The group content types that are associated with each group (so the site admin knows which content types to select in the Linkchecker configuration)
* The group roles and whether or not each role has access to the group broken links report
* The number of broken links in each group (not including links in comments)

Requirements

Linkchecker
Organic Groups

Project Page

http://drupal.org/sandbox/caesius/1801426

Git

git clone --recursive --branch 7.x-1.x http://git.drupal.org/sandbox/caesius/1801426.git og_linkchecker

Version

Drupal 7

Reviews

Review #1
Review #2
Review #3

Comments

rico.schaefer’s picture

rico.schaefer commented

Hi,
can you check the following:

  • Add function doc comment for every function.
  • og_linkchecker.admin.inc: All inline comments must end in full-stops, exclamation marks, or
    | | question marks
  • Remove spaces before function calls in og_linkchecker.module

Anything else is looking fine.
Regards,
Rico

rico.schaefer’s picture

rico.schaefer commented
Status: Needs review » Needs work
caesius’s picture

caesius commented

Done. The remaining code standards errors are deliberate (no function doc for hooks and newline after inline comments).

caesius’s picture

caesius commented
Status: Needs work » Needs review
caesius’s picture

caesius commented
Issue tags: +PAreview: review bonus

Adding review bonus tag.

tglynn’s picture

tglynn commented
Status: Needs review » Needs work

Online PAReview has insignificant and very few errors, as expected based on your earlier comment.

Topic of the module looks original, as compared with currently available modules.

Lines 11 through 20 could use some work. I would like to see the alert message within a translation function and see the database query be using the database API. Writing SQL is quick but many people are moving to databases such as MongoDB that will not automatically work with SQL statements such as this.

The main change that needs to be addressed:

• I get the white screen of death on /admin/reports/og_linkchecker

Overall, I think it’s written well but I’d feel more comfortable seeing that the back end screens are working so that the end functionality is good.

caesius’s picture

caesius commented
Status: Needs work » Needs review

Ouch... I introduced an extra comma when I refactored some code to comply with coding standards. Whoops :|

format_plural is a translation function by the way.

Other than that I updated the db_query to use the database API so it should be ready for another go.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Needs work
Issue tags: -PAreview: review bonus +PAreview: security

manual review:

  1. You have to use "/**" instead of "/*" for doxygen comment blocks on functions. See http://drupal.org/node/1354#hookimpl . That is why pareview.sh complains.
  2. "module_load_include('module', 'og');": no need to load module files in your module file. All enabled module files will be loaded by Drupal automatically in a request.
  3. Do not include other files globally, include them in functions when yo actually need them.
  4. "check_plain($node->title . ': Linkchecker Report')": all user facing text must run through t() for translation. You can use "@" or "%" placeholders with t() that will run check_plain() for you.
  5. "$content .= "<p>$data->name ($data->type)</p>";": a content type label is user provided input and needs to be sanitized before printing to avoid XSS exploits. See http://drupal.org/node/28984 . Same for the role name.
  6. "'group' => t('!title (!type) [<a href="!report">report</a>]',": Same here: title is user provided input and should use the "@" or "%" placeholder.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

caesius’s picture

caesius commented
Status: Needs work » Needs review

All fixed. PAReview gives only the two remaining deliberate errors re: blank lines after inline comments. Thanks!

cubeinspire’s picture

cubeinspire commented
Status: Needs review » Needs work

Hi,

1. I don't think it is a reason to block the review because of that but on lines 19, 20, 31 (module file) etc... there are links inside the t() function.
It wouldn't be better to concatenate that ?
t('some text with a ') . l('link', 'admin/help/linkchecker') . t(' and some more text after');

2. On those same lines there is a sanitation on a string that is not provided by the user. That's not needed.
$output .= '<p>' . t('Note: Links are evaluated only for content types selected in the main <a href="@linkchecker-conf">Linkchecker configuration page</a>.', array('@linkchecker-conf' => '/admin/config/content/linkchecker')) . '</p>';

3. Its quite ironic, but the links on the help text are broken if clean url is not enabled.
You should also add the global $base_path to the href of those links.
Some Drupal installations are not on the "/" path (as mine).

I don't see any security issue here.

caesius’s picture

caesius commented
Status: Needs work » Needs review

@logicdesign,

Regarding putting link anchors in translatable text, refer to this page, since it advises against what you suggest in #1 and #2: http://drupal.org/node/322774

Regarding #3, I wrapped my internal URLs in the url() function, so they should work without Clean URLs now.

cubeinspire’s picture

cubeinspire commented
Status: Needs review » Reviewed & tested by the community

Thank you for the link.
I was looking for that page but couldn't find it.
This is looking good now, I think it is RTBC.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Reviewed & tested by the community » Fixed

Thanks for your contribution, caesius!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Adding reviews for review bonus