Closed (fixed)
Project:
Anonymous Suggestion Box
Component:
Code
Priority:
Normal
Category:
Bug report
Assigned:
Reporter:
Created:
15 Aug 2014 at 04:11 UTC
Updated:
28 Sep 2014 at 12:00 UTC
Jump to comment: Most recent
First of all, great start! I took a peek, and have a few action items.
module:
install:
Comments
Comment #1
benjaminarthurtAwesome! Thanks for the review. I'll get started fixing things soon.
Comment #3
benjaminarthurtanonymous_suggestion_box_permission - permission machine names should be in lowercase.Fixed is now "submit anonymous suggestions"anonymous_suggestion_box_permission - "Access Anonymous Suggestion Box" - semantically, should probably be "Submit anonymous suggestions"Fixed is now "submit anonymous suggestions"anonymous_suggestion_box_menu - don't use a variable for a Menu item name; it can always be changed using the menu system.Fixed, changed to "Anonymous Suggestion Box" and removed corresponding field from Admin form.anonymous_suggestion_box_form_submit - This can be simplified. If email, email. If db, write to db. Don't need explicit if, else in this case.Fixed, I really love overcomplicating things :)anonymous_suggestion_box_admin_validate - Use drupal_strlen to count characters - https://api.drupal.org/api/drupal/includes%21unicode.inc/function/drupal...No longer needed since it was only used to validate the TitleComment #7
benjaminarthurtanonymous_suggestion_box_view - Add a pager - https://www.drupal.org/node/262854 - or consider using Views instead.Added views, supplied default view (page and block), default view is enabled by default, menu path set to replace default page with-in module. Left default display page intact as a fallback if views not enabled (although seriously, why not Views are awesome). I should eventually edit the default module display page so that it has a pager or some sort of searchable something or other.Observations and suggestions can only be 255 characters long? Anonymous suggestions can get verbose, consider using a bigger field :-)fixed - now using text big for all 3 fields.Delete variables in hook_uninstall.fixed...No reason to index location, observation, or suggested. This will not improve performance; if anything, it will create overhead.Fixed.Comment #8
benjaminarthurt