Foursqure Tips

This seems like an interesting tool, Especially if you had lat/lon as a parameter to the block and it pulled in tips from the lat/lon on a node it could be really useful.

I found a few issues. I consider the function and variable naming to be critical items since they are likely to cause serious bugs in some scenarios. The rest of the problems are more about style than requirement, but they are still important to fix.

The standard for namespacing Drupal functions is to use yourmodulename_functioname. So, getTips should be foursquare_tips_getTips. It's also standard to only use camelCase if you are using objects which this module doesn't, so foursquare_tips_get_tips() would be more standard.

Similarly, all of the variables in the module need to be prefixed with "foursquare_tips" because using a variable_get/variable_set for something named "token" is really likely to conflict with other modules.

The getTips function makes a direct call to foursquare's server. The Drupal block caching system means that this will be cached per role (which is the default) but you may want to cache longer than that.

The getTips function has two purposes: first to get the tips and then to format them. I suggest breaking it apart by adding a theme function for the second set of tasks.

    case 'list':
  19             $blocks[0]['info'] = t('Foursqure Tips');
  20             return $blocks;
  21         case 'view':
  22             $blocks['subject'] = t('Foursqure Tips');
  23             $blocks['content'] = getTips();
  24             return $blocks;

Your case 'view' on the blocks assumes that it will always be block $delta 0. You should check the delta so future additions of blocks will work.

You've got the permission:

But that permission is not descriptive enough and is not defined by a hook_perm. It seems you want to use "change FoursqureTips configuration" and I suggest testing the module as a user other than UID 1.

The time_ago function can be replaced with http://api.drupal.org/api/drupal/includes--common.inc/function/format_in...

Your info file has extra carriage returns in it.

Great progress! I encourage you to look into format_interval a bit more and state specifically what isn't working. This is an opportunity to learn.

A couple problems remain:

You've got the permission:
array('change message'),

But that permission is not descriptive enough and is not defined by a hook_perm. It seems you want to use "change FoursqureTips configuration" and I suggest testing the module as a user other than UID 1.

Still not addressed.

Throughout the files you seem to be using tabs or 4 spaces where it should be using 2 spaces.

It would be best to filter the text returned from the foursquare server to prevent XSS. Unless they absolutely need to use javascript, you can use either check_plain or filter_xss on the text. See this text filtering cheat sheet for advice on which function to use and when.

Your tpl.php uses ID values. An ID should only exist once on a page, but the foreach loop makes me think many of these will appear multiple times. Classes seem better in at least some of the cases.

The css classes would be best prefixed as well: tipscontainer should be foursquaretipscontainer (or maybe foursquare-tips-container).

Um, this seems to be a copy of the code from another sandbox application: #1078352: Foursquare Tips that was filed before yours.

I think this is a "won't fix" based on being a duplicate module.

Ok, that makes some sense.

You still need to address the problems I mentioned in comment #5 and should look to address the issues raised in the issue linked in comment #6.

3. There is multiple ID's in tpl.php because its implemented that way. Please check a flow.

I don't understand. The same ID cannot be used multiple times in a single document. See http://www.w3.org/TR/html401/struct/global.html#h-7.5.2

You are still using classes that are too generic in the tpl.php. 'tips' is not a great since it could clash with helptips

Hard-coded styles should also be moved to your css file.

Adding tag to indicate this module had security issues.

I just did one more look over the code and noticed that the menu item has no access arguments nor callback.

So, http://example.com/admin/settings/foursqure_tips/settings is not available to any users. I also suggest shortening it to just admin/settings/foursqure_tips - there's no need for the last /settings on that url.

You also haven't addressed all the issues in http://drupal.org/node/1078352#comment-4348690 - please look into those.

Please also look for some form of caching. The foursqure_tips_get_tips function currently makes an API request on every page load. I'm sure this would massively slow down sites. This article from lullabot is a guide to caching data that may be helpful.

While overall my comment is full of places for more work I want to say thanks for sticking with this process. It seems like a useful module and with a few more improvements I feel like it will be ready to go.

Your fix to the menu code does not work the way you seem to hope it will work. The hook_menu is evaluated when the menu is rebuilt, not dynamically. You should move a global $user into the access callback.

But really, you are using the menu/permission system wrong. I suggest declaring a real permission and using that instead.

It seems like there are some old code style issues and you've created some new ones. Please be sure to run the module through coder's review before each commit and familiarize yourself with http://drupal.org/coding-standards

This is not translatable:
$output="Cannot Connect to Foursqure";

Your use of format_interval was probably better than time_ago. The advice from the other comment was to move it to a preprocess which you didn't do. Please do that.

The applicant has been contacted to ask if the application is abandoned.

The application has been closed. If you would like to reopen it, you are free to do so.
See http://drupal.org/node/894256

Nice idea.

Some coding errors:

• There are no readme.file
• You are working in the master branch, see this: http://drupal.org/node/1127732
• Ran your code through the ventral.org checker, and it found lots of errors, so check them out: http://ventral.org/pareview/httpgitdrupalorgsandboxmobifilia1117718git

Other than that it looks ok, got it to work. Expected the tips to be clickable, i think that would have been nice.

You are still working in the master branch, according to the automated ventral.org check.

You also have lots of errors in your code, check the link above and go through your code.

Great that you will add clickable tips :)

Hi
Have you checked this link? http://ventral.org/pareview/httpgitdrupalorgsandboxmobifilia1117718git

It gives you a summary of the errors in your code and other files missing.

Very good, i see one error because the branch is named 7.x.1.x, it should be 7.x-1.x.

Other than that i got your module to work good, and did not see anything wrong.

I'm no expert in Git, but i would think there is some way to rename them. Or you could create a new branch and remove the old one.

Maybe point 6 here can help you: http://drupal.org/node/1127732

I still see there are issues with the branches when i check the automatic test. I believe you have to resolve them before you can get accepted. Unfortunatly i don't know how to fix it, so you should wait for someone else to help you with this.

Hi mobifilia,

Looking through your code i found the following:

/**
 * Implements of hook_block()
 */

$path = drupal_get_path('module', 'foursqure_tips');
drupal_add_css($path . '/css/forsqure_style.css');

in foursqure_tips.module on line 11 >> 16, it seems to me you should use hook_init for the above 2 lines.

In the .info file the last character is a newline (\n) you can remove this.

name =Foursquare Tips
description =Shows Latest Tips from Foursquare.
core = 7.x
\n

Beside this, i would define a package here. Perhapse:

name =Foursquare Tips
description =Shows Latest Tips from Foursquare.
core = 7.x
package = Foursquare

stylesheets[all][] = css/forsqure_style.css

You can also add the css file which is now added in the .module file here. see above:

Also, i would advise you to insert some inline comments, especially the foursqure_tips_get_tips function is rather large without any inline comments. Remember that this can also help you remember why you programmed it this way when you look back at it some time from now.

Next, you define some variables in your .module file. Since there is no .install file this would mean that after uninstall these variables would still be defined, these should be deleted with hook_uninstall.

I cannot test the module because foursquare is blocked by our company web-filter, assuming this will work it looks like a usefull module.

Hi mobifilia,

My points are indeed fixed, good job.

On a different note, your README.txt is quite small.
I like to look at this one: http://drupal.org/node/1271064

I would devide the readme in:

Introduction (Who helpt you, where can we find your profiles - not really required)
Features (What features does the module have)
Usage (How can we use the module)
Installation (Even though most of us can do it with our eyes closed, try to make this as easy as possible for beginners)

As i could not test the module i will not set the status to reviewed & tested by the community. I would recommend to have a look at the readme, for now i will let the status of this issue remain @ needs review.

Hi again :)

Now the module looks good.

According to this: http://drupal.org/node/542202 You should stick to the package names already in Drupal,
and not invent too many new ones.

Fix it to something more appropriate and i think it is good to go :)

Very good, i could not see anything wrong.

One thing i would like to see is an option to disable your CSS, if i want to implement it in my theme i either have to override everything or edit your module.

I'm setting the status to "reviewed and tested" :)

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

One comment ... I would recommend against including a default FourSquare oAuth token inside the code ... instead, you should have users supply their own through an admin configuration page.

Thanks for your contribution, mobifilia!

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:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

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.