Untappd Menu Boards is a plugin for the Bean Module (https://drupal.org/projects/bean) to provide a custom Bean type which displays information from the Untappd API for Businesses.

The plugin module also provides theme-able templates to allow users to customize menu boards by location or by specific menu to tailor the display to their needs.

Manually reviewed issues

https://www.drupal.org/project/projectapplications/issues/3068991#commen...
https://www.drupal.org/project/projectapplications/issues/3047176#commen...
https://www.drupal.org/project/projectapplications/issues/3064269#commen...

Project link

https://www.drupal.org/project/untappd_menu_boards

Git instructions

git clone --branch 7.x-1.x https://git.drupalcode.org/project/untappd_menu_boards.git

PAReview checklist

https://pareview.sh/pareview/https-git.drupal.org-project-untappd_menu_b...

Comments

oranges13 created an issue. See original summary.

oranges13’s picture

oranges13
she/her
Location Michigan
commented
Issue summary: View changes
oranges13’s picture

oranges13
she/her
Location Michigan
commented
Issue summary: View changes
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Issue summary: View changes
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Issue summary: View changes
oranges13’s picture

oranges13
she/her
Location Michigan
commented
Issue summary: View changes
oranges13’s picture

oranges13
she/her
Location Michigan
commented
Issue summary: View changes
Issue tags: +PAreview: review bonus
vuil’s picture

vuil
Location Bulgaria 🇧🇬 🇪🇺 🌍
commented
Status: Needs review » Needs work

Thank you for the contribution!

Please fix all issues mentioned on Pareview checklist page as the following one:

FILE: ...b/vendor/drupal/pareviewsh/pareview_temp/untappd_menu_boards.install
--------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------
 1 | ERROR | [x] Missing file doc comment
 1 | ERROR | [x] The PHP open tag must be followed by exactly one blank
   |       |     line
 7 | ERROR | [x] Expected 1 blank line before function; 2 found
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------
oranges13’s picture

oranges13
she/her
Location Michigan
commented
Status: Needs work » Needs review

Hi thank you, this has been updated.

https://pareview.sh/pareview/https-git.drupal.org-project-untappd_menu_b...

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Needs work

Thanks for you contribution!

UntappdMenuBoards::view(): You are retrieving external data and passing it on to the template without sanitization for XSS. One could argue that the external Untapped API source is trusted, but we should not rely on that. Therefore we should consider the retrieved data as untrusted user provided input that we need to sanitize. You are printing names without sanitization as far as I can see for example. I'm not sure if we should qualify this as direct security vulnerability since an attacker would have to get malicious data into the API, not sure how feasible that is :-)

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs work » Closed (won't fix)

If you are still working on this application, you should fix all known problems and set the status to Needs review. (See also the project application workflow.)
Please don't change status of this application if you aren't sure you have time to dedicate to this application, or it will be closed again as won't fix.

oranges13’s picture

oranges13
she/her
Location Michigan
commented
Status: Closed (won't fix) » Needs review

@klausi -- Technically not impossible, but I would think it's extremely improbable based on several factors:

  1. They would need to know that our site was utilizing the Untappd API to display this data.
  2. They would need to have the credentials to log into the Untappd Business Dashboard (not a standard Untappd user account, they would need one of the administrators accounts).
  3. They would have to insert data into the API that would cause an XSS attack, given that Untappd doesn't already sanitize for this on their end.

I added filter_xss to the output array.

oranges13’s picture

oranges13
she/her
Location Michigan
commented

Another thing I have run into. According to Parreview, I need to fix these errors:

FILE: .../drupal/pareviewsh/pareview_temp/plugins/UntappdMenuBoards.class.php
--------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------
  45 | ERROR | Type hint "array" missing for $form
  45 | ERROR | Type hint "array" missing for $form_state
 126 | ERROR | Type hint "array" missing for $content
--------------------------------------------------------------------------

However, when I add those hints, I receive a 500 error as follows:

Fatal error: Declaration of UntappdMenuBoards::form($bean, array $form, array &$form_state) must be compatible with BeanPlugin::form($bean, $form, &$form_state) in /var/www/drupal-7.43/sites/all/modules/untappd_menu_boards/plugins/UntappdMenuBoards.class.php on line 206

It seems like the Bean Module does not have the same function signature, and that results in an error. What should I do in this case?

For anyone else facing a similar issue, this was because I had included detailed docblocks before the form and view functions. None of the other Bean plugins included detailed docblocks, which set off the need for the typing within the function signature, which then caused the incompatability.

So just keep your docblocks in the BeanPlugin class simple and everything will be happy.

This is fixed and ready for review. Thank you!

klausi’s picture

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

Thank you!

untappd_menu_boards.tpl.php: "Last Updated: ": all user facing text must run through t() for translation.

Otherwise looks good to me!

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Assigned: Unassigned » avpaderno
Status: Reviewed & tested by the community » Fixed

Thank you for your contribution! I am going to update your account.

These are some recommended readings to help with excellent maintainership:

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the dedicated reviewers as well.

Status: Fixed » Closed (fixed)

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