[D7] Manage Advertisements

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Automated Review

OK

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

No: Does not follow the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. Suggestion : You can add multiple images fields option for advertisement type.
2. (*)Remove admin theme selection because if user select the admin theme and content region then content will display on admin section

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Automated Review

Issues identified by pareview.sh seems to be clean.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication or fragmentation but please clearly justify the difference between these modules:

• ad_manager
• simpleads

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

No: Does not follow the guidelines for in-project documentation or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

1. (+) You should use manage_advertisements.admin.inc instead of manage_advertisements.inc
2. (+) There should be Implements hook_settings() instead of Implements manage_advertisements_settings() in manage_advertisements.module file at line no 78.
3. (+) As of Drupal 7, a call to drupal_uninstall_schema() is no longer necessary. See "A module no longer should explicitly install or uninstall its database schema in hook_install() or hook_uninstall()."

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

OK then changing status to Need Review.

Automated tests passed successfully.

Manual review:

• Clearly formatted README file with good explanation. This passes for me.
• If I choose 'text' as a format, I am unable to choose my text format filters. Say for example if I want to attach a WYSIWYG editor.

Baseline: good and efficient small module to quickly create ads. Looking forward to see this become more extended in the future.

Removing PAReview: Single project promote because module has enough code.
As all reviewers are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

@Shashwat Purav, I'm just mentioning that if I want to add a WYSIWYG editor to that body field on the ad for only a certain role, I can't.
On the other hand, it's your design. You can pick up my mention as a feature request.

Automated Review

[Best practice issues identified by pareview.sh / drupalcs / coder. Please don't copy/paste all of the results unless they are short. If there are a lot, then post a link to the automated review and mention that problems should be addressed.]

Manual Review

Individual user account

[Yes: Follows / No: Does not follow] the guidelines for individual user accounts.

No duplication

[Yes: Does not cause / No: Causes] module duplication and/or fragmentation.

Master Branch

[Yes: Follows / No: Does not follow] the guidelines for master branch.

Licensing

[Yes: Follows / No: Does not follow] the licensing requirements.

3rd party assets/code

[Yes: Follows / No: Does not follow] the guidelines for 3rd party assets/code.

README.txt/README.md

[Yes: Follows / No: Does not follow] the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

[Yes: Follows / No: Does not follow] the guidelines for project length and complexity.

Secure code

[Yes: Meets the security requirements. / No: List of security issues identified.]

Coding style & Drupal API usage

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. Just a recommendation
2. For Text advertisement You can mention as TEXT/HTML as it works with html as well.
3. If advertisement content like image tag has width more than the block region it breaks the ui. Maybe you can keep a wrapper and set overflow hidden.
4. (+)When Manage Advertisements module is disabled the ads created are not removed.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Automated Review

Looks Fine

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

1. (*) I am able to see Text format section even if i select Advertisement type as image or video.
2. (+) While adding a advertisement notice was generated after submited. I have attached the notice image.
3. Just a recommendation : If adding an image advertisement and i provide specific images dimension it just resize through html width and height,thus image look shrink. Why dont you resize the image in given dimension.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walk through are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Hi,

Manual review:

manage_advertisements.module

• Why do you use $form seems it should be a $form_state (148-149, 164-165, 194, 210-211)
• On advertisements edit form you are use hidden textarea field for image and video block this is strange solution.
• In your table 'manage_advertisements' the column 'updated' is empty for text blocks
  On manage_advertisements.js file you are use .on() on line 9. But default drupal 7 installation use jquery 1.4 and .on() wouldn't work because it is available since jquery > 1.7

Wishes

• Added image is always wrapped in

  element. It can be not good for displaying.

• I can't add image without a link.

Change status

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.

manual review:

1. manage_advertisements_settings(): don't call theme() here, just return a nested render array. Drupal core will render it later for you. See https://www.drupal.org/node/930760
2. manage_advertisements_settings(): "'#value' => 'Execute',": all user facing text must run through t() for translation. Make sure to check all your strings.
3. manage_advertisements_settings_submit(): if you delete from the block table you must also make sure that you don't delete block from other modules, right? Why is there no condition for the "module" column in the block table? Please add a comment.
4. There is a security vulnerability in this module and as part of our git admin training I'm assigning this to joachim so that he can take a look. If he does not find anything I'm going to post the vulnerability details in one week. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

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

/**
 * Implements custom function manage_advertisements_enabled_themes().
 */
function manage_advertisements_enabled_themes() {

It looks like you've labelled every function as 'Implements...'. Only functions which are a hook implementation are implementations ;)

  $form['manage_advertisement_create_advertisement'] = array(
    '#markup' => l(t('New Advertisement'), 'admin/config/content/manage_advertisements/settings/create_advertisement', array('attributes' => array('class' => array('button')))),
  );

Use the local tasks system to create the link rather than hardcoding it into the form.

    $form['page_region_settings']['create_advertisement_region_' . $enabled_theme] = array(
      '#type' => 'select',
      '#title' => $enabled_theme,
      '#options' => $regions,
    );

Use the theme's label, not its machine name.

  $form['cancel'] = array(
    '#markup' => l(t('Back'), 'admin/config/content/manage_advertisements/settings', array('attributes' => array('class' => array('button')))),
  );

This shouldn't be styled as a button -- it doesn't submit the form. Also, people can go back with their browser controls, or with the breadcrumbs. AFAIK we only provide buttons to get out of destructive operation forms (such as delete).

When I submit the edit form, I get a crash:

> PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'bartik-manage_advertisements-1' for key 'tmd': UPDATE {block} SET module=:db_update_placeholder_0,

  $form['operations'] = array(
    '#type' => 'select',
    '#options' => array(
      0 => t('- Choose an operation -'),
      'delete_advertisements' => t('Delete Advertisements'),
    ),
  );

This is overkill if there is only one operation.

    $options[$block->advertisement_bid] = array(
      'advertisements_name' => $block->advertisement_description,
      'advertisements_type' => $block->advertisement_type,
      'edit' => l(t('Edit'), 'admin/manage_advertisements/' . $block->advertisement_bid . '/edit', array('query' => array('destination' => 'admin/config/content/manage_advertisements/settings'))),
      'delete' => l(t('Delete'), 'admin/manage_advertisements/' . $block->advertisement_bid . '/delete', array('query' => array('destination' => 'admin/config/content/manage_advertisements/settings'))),
    );

Here's the security issue I found. You are not sanitizing the ad description, which is user-entered text. A malicious user with access to create advertisements could enter JS code here, for instance.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.