[D6] Banckle Meeting

Welcome,

please get a review bonus first. Then try to fix issues raised by automated review tools: http://ventral.org/pareview/httpgitdrupalorgsandboxmasoodanwer1717534git

Manual review

1.1 Ensure your application contains a repository and project page link.
Check
1.2 Ensure your project is not a duplication.
Isn't it the same as here : http://banckle.com/wiki/display/onlinemeeting/integration-with-drupal-6....
1.3 Ensure you don't have multiple applications.
Didn't found any.
2. Basic repository checks

2.1 Ensure the repository actually contains code.
Check
2.2 Ensure you are working in a version specific branch.
Please create an branch . You are working on the master branch
2.3 Ensure your project contains a minimum of handwritten code.
Seems to be so.
3. Security Review

3.1 Ensure the project does not contain any security issues.
Seems ok
4. Licensing checks
4.1 Ensure the repository does not contain a ‘LICENSE.txt’ file.
Please remove this file.
5.1 Ensure the project page contains detailed information.

Looks ok.
5.2 Ensure the repository contains a detailed README.txt.

Contains and looks nice. Maybe not enough documentation ?
5.3 Ensure the code contains a well-balanced amount of inline-comments.

Could use some documentation (Found not really much docs in the .module.)
Automated review:

Please check these issues: http://ventral.org/pareview/httpgitdrupalorgsandboxmasoodanwer1717534git

7.1 Ensure you are using Drupals API correctly.

Looks ok.

Hi,

here is report of manual review of your code:-

1) "'access arguments' => array('access administration pages'),": The administration menu callback should probably use "administer site configuration" - which implies the user can change something - rather than "access administration pages" which is about viewing but not changing configurations
2) Please make sure you do proper formatting of your code as mentioned here
3) In line 114 you should not use html part that should be done in tpl files only. Also your variable_get('banckle_meeting_widget_code', '') should be variable_get('banckle_meeting_widget_code', NULL)

Thanks
Parwan

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

If you reopen this please keep in mind that 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 :-)

Hello,

Sorry for the inactivity of the project, but we've been having some team adjustments. Now we've resolved all the issues mentioned in the review and hosted the repo on github (https://github.com/banckle/Banckle-Meeting-for-Drupal) for your review.

So we want to reopen this project, can you please make me "contributer/commiter" for this project.

Thanks

@banckle - please note that organization accounts cannot be approved for git commit access. See #1966218: Infrastructure to support organization accounts and #1863498: Create Basis for ToS to allow organizations to share accounts for details on what is/isn't allowed.

Greg,

Thanks for your reply, but instead of having commit access with an orginzations account, i really dont need that. Just give me rights to my personal account "assadmahmood". I'll appreciate your quick action.

Thanks,
Assad Mahmood

we've resolved all the issues mentioned in the review and hosted the repo on github

The changes need to be made in the project on drupal.org in order to continue the project application process.

The changes need to be made in the project on drupal.org in order to continue the project application process.

to make changes to the project i need commit rights.

You need to ask the sandbox owner to grant you commit rights or fork your own sandbox.

Hello,

Thanks for your reply. Changes were committed to drupal.org. Please proceed with the project application process.

Thanks.

This issue is not fixed? See https://drupal.org/node/532400

Master Branch

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

access administration vs. administer site configuration

The administration menu callback should probably use "administer site configuration" - which implies the user can change something - rather than "access administration pages" which is about viewing but not changing configurations.

• You have a large number of syntax/style issues reported here: http://pareview.sh/pareview/httpgitdrupalorgsandboxmasoodanwer1717534git, please try to get that number down.
• Use single quotes wherever possible - its faster and matches our coding standard. see https://drupal.org/coding-standards#quotes
• In your theme functions you don't need to check for type module and your modulename, just register your module's theme function. I'm not sure why you have 2 functions - since your module is named banckle_online_meeting, only banckle_online_meeting_theme() will be called
• Using banckle_online_meeting_page_alter() is very heavy, this will be executed for every Drupal page request. Can you use hook_block_info() and _view() to create a new block for your widget? This would allow the admin the assign it to whatever region they like.
• If your module requires curl to be installed, please implement a hook_requirements in your .install file
• All the code in banckle_online_meeting.tpl.php is not appropriate for a template file - you should do that in your module, the template file is for themers and should contain only simple variables if possible
• banckle_online_meeting_invalid_auth_error.tpl.php and the other error templates are not template files - could you just use a theme function instead to return this text instead? Or store the text in a variable and retrieve it with variable_get().

----
Top Shelf Modules - Crafted, Curated, Contributed.

Thanks for your valuable feedback, we are working on to fix things as you mentioned. will update you soon.

While I'm fixing all the issues you have mentioned, can you please review it once, because i've almost fixed all the ERRORS which were being generated through pareview.sh

Thanks.

Many of the problems in #15 are still present. Please fix them before setting it back to needs review.

Additionally, there appears to be an XSS vulnerability via the $widget_code, $widget_logo, $widget_width, $widget_height in the banckle_online_meeting_page_alter function. You should either validate and filter those values before including them in an iframe url or change the permission from "access administration pages" to something appropriate which is listed on this page or do both filtering and the permission change.

Issues fixed

1) Now code is on a D7 branch
2) Maximum number of syntax issues were resolved
3) Theres only one theme function now and we are not checking module type and name anymore
4) we are no more using banckle_online_meeting_page_alter instead we have implemented hook_block_info and hook_block_view
5) modified hook requirement in our .install file
6) banckle_online_meeting.tpl.php is no more required so its removed.
7) banckle_online_meeting_invalid_auth_error.tpl.php isn't required so its removed too.

Please review it again.

The D7 branch should be named 7.x-1.x, and the master branch should be removed. These docs should cover that:

• Moving from a master branch to a version branch.
• release naming conventions
• creating a branch in git

Thank you for including an INSTALL.txt file
Using single quotes is still a good idea - but not a blocking issue.
In banckle_online_meeting_block_view(), instead of calling banckle_online_meeting_form() with dummy values, instead just use the variables directly. Like

  $widget_code = variable_get('banckle_online_meeting_widget_code', NULL);
  $widget_width = variable_get('banckle_online_meeting_widget_width', 180);
  $widget_height = variable_get('banckle_online_meeting_widget_height', 430);
  $widget_logo = variable_get('banckle_online_meeting_show_logo', 'true');

You still have calls to theme("banckle_online_meeting_invalid_auth_error") and banckle_online_meeting_network_error and banckle_online_meeting_no_auth_error. Now that you got rid of those files (which is good), I think those will all return null. How about just returning the text string that used to be in those template files?

This module seems to assume that the user/pass combo will be stored in $user->data, but it doesn't provide any way to insert that data. Could you add something to the README that tells the admin how they should handle this?

Getting better though!
----
Top Shelf Modules - Crafted, Curated, Contributed.

Thanks for the review. All the recommended modifications were implemented

1) Master Branch was removed and branch name was changed.
2) No more using banckle_online_meeting_form() instead using what you recommend.
3) No more reference to theme("banckle_online_meeting_invalid_auth_error") since we are not using that particular area anymore
4) Readme file was updated with usage info.

Please review asap.

Thanks.

Hmm, I'm not sure what happened, but http://drupalcode.org/sandbox/masoodanwer/1717534.git shows 2 branches:

• 7.23-1.0
• master

Neither of which is right. You should have a single branch named 7.x-1.x. Let us know if you need help with that.
You also still have the menu item left for admin/config/banckle/onlinemeeting/dashboard, which I don't think will work anymore since the page callback function was removed.

----
Top Shelf Modules - Crafted, Curated, Contributed.

Thanks for you reply kscheirer, I've removed the dashbaord menue item, but for some reason i'm not able to remove the master branch. However you can check the changes on current one "http://drupalcode.org/sandbox/masoodanwer/1717534.git/shortlog/refs/head..." while i'm trying to remove the master.

I've set the 7.23-1.0 as default branch. but one confusion is still have when you say that name shouldbe 7.x-1.x do i need to replace x with the appropriate number or you mean it literally?

Yes, the branch name should be exactly 7.x-1.x without any substitutions. When you create a release, that's when you can tag your code as 1.0, and Drupal will package up a 7.x-1.0 release for you. We never use 7.23 - it's always 7.x to signify that this module works with all versions of Drupal 7.

----
Top Shelf Modules - Crafted, Curated, Contributed.

I've pushed the code to 7.x-1.x please have look.

Looks good! One more step, remove the master branch and the 7.23-1.0 branch please.
In your Install.txt file, I don't think you're requiring cURL anymore.
See if you can resolve some or all of the style errors reported here: http://pareview.sh/pareview/httpgitdrupalorgsandboxmasoodanwer1717534git.

----
Top Shelf Modules - Crafted, Curated, Contributed.

Branch is removed, and install.txt file is also updated.

Still have a 7.23-1.0 branch? Can you remove that as well?

----
Top Shelf Modules - Crafted, Curated, Contributed.

Its removed already, please do the needful. Thanks.

You still have a lot of style issues reported here: http://pareview.sh/pareview/httpgitdrupalorgsandboxmasoodanwer1717534git. Nothing looks like a blocker though. It's been a month without any problems reported, so I'm promoting this myself as per https://drupal.org/node/1125818.

Thanks for your contribution, masoodanwer!

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.

----
Top Shelf Modules - Crafted, Curated, Contributed.