CVS edit link for wiifm69

I am currently developing a drupal e-commerce website, as a part of this, I am creating a new theme. I am looking to give this theme back to the drupal community and license it GPL.

For an HTML mockup of the homepage, please see http://dl.dropbox.com/u/759954/kiwikanvas/index.html - note this is only the homepage, and is by no means feature complete ;)

Future theme improvements:
* integration with the color module
* graceful degradation for none awesome browsers (rgba and other CSS3 technologies are used in this theme)

Hope to hear from you soon

Thanks
Sean

CommentFileSizeAuthor
#17 statistics_ajax-2010.06.02.tar_.gz2.77 KBwiifm
#14 statistics_ajax-2010.05.30.tar_.gz2.89 KBwiifm
#7 statistics_ajax.tar_.gz8.26 KBwiifm

Comments

wiifm’s picture

wiifm
Primary language English
Location Wellington, NZ
commented
Component: Miscellaneous » User interface
Status: Postponed (maintainer needs more info) » Needs review
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Component: User interface » Miscellaneous
Issue tags: +Theme review
wiifm’s picture

wiifm
Primary language English
Location Wellington, NZ
commented

beta site is live, visit http://kiwikanvas.co.nz for my current progress. Any chance CVS access could be granted?

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Postponed (maintainer needs more info)

If you would have read Apply for contributions CVS access, you should know what you need to do.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Postponed (maintainer needs more info) » Closed (won't fix)

There have not been replies from the OP in the past 7 days. I am marking this report as won't fix.
I am rejecting the CVS application on the basis that the OP didn't carefully read Apply for contributions CVS access.

wiifm’s picture

wiifm
Primary language English
Location Wellington, NZ
commented
Status: Closed (won't fix) » Needs review
Issue tags: -Theme review +Module review
StatusFileSize
new statistics_ajax.tar_.gz8.26 KB

I am reapplying for CVS access to contribute my first drupal module

Introducing 'statistics_ajax' (from the readme):

********************************************************************
DESCRIPTION:

This module provides a way to programmatically update the {node_counter}
table (provided by the core statistics module).

This module listens to a URL in the format of '/statistics/ajax/[nid]'
where [nid] is the id of the node you are looking to update the statistics
for

The response is plain text in the format:

'statistics_ajax_success' = Success, the database update was successful

'statistics_ajax_error [error]' = Error, the database was not updated,
the [error] text will be more descriptive

********************************************************************

Module uploaded in tar.gz form, more information in the readme inside.

Module can be seen in action (by seen, I mean well if you look at firebug's 'net' panel), if you visit http://kiwikanvas.co.nz/browse/popularity and click on any of the photo thumbnails in the main column

Thanks
Sean

wiifm’s picture

wiifm
Primary language English
Location Wellington, NZ
commented

Anyone care to have a look at this module? Many thanks, Sean

zzolo’s picture

zzolo commented
Status: Needs review » Needs work

Hi @wiifm69, sorry for the delay. I am confused on why your module does what it does? What is the use case?

--
Note: Please be patient with the CVS application process. It is all done by volunteers. Our goal is not to be arbitrarily slow or meticulous. Our goal is to get you CVS access and ensure that you are and will become a more responsible Drupal contributor. For a quick reference on what I look for in a code review, please see this article.

wiifm’s picture

wiifm
Primary language English
Location Wellington, NZ
commented

Hi @zzolo,

No worries for the delay, I do appreciate all the work you volunteers put into this ;)

I will try to explain in more detail what this module does:

I run a photography business, in which customers can browse my site and see selected artists work on display. Core statistics worked beautifully when the end users would visit the actual node in question (thus updating the {node_counter} table), but most if not all of my content is browsed in 'sets'. Sets can be anything from taxonomy terms, to searching, to views output. As there is no need to actually visit the node in order to purchase the product (or get a closer look at the photo), core statistics will never reflect the most popular content.

So basically I wanted a way to reflect that the most popular content on the site as being the most clicked on photographs.

I now have a view ( http://kiwikanvas.co.nz/browse/popularity ) that sorts all content from highest views to lowest, as this works exactly as needed.

This module development started with a question on stackoverflow.com ( http://stackoverflow.com/questions/2482241/update-node-counter-table-pro... )

If you need more information, let me know

Many thanks
Sean

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs work » Needs review

I take you want to know the most popular content basing on the number of times a picture has been seen.

wiifm’s picture

wiifm
Primary language English
Location Wellington, NZ
commented

Exactly correct, a user may not necessarily visit the full node, but they may click on the image associated with the node

zzolo’s picture

zzolo commented
Status: Needs review » Needs work

@wiifm69, thanks for the application and patience. This makes sense, just didn't think about it enough.

The following points are just a start and do not encompass all of the changes that may be necessary for your approval. Also, a specific point may just be an example and may apply in other places.

  1. No need for LICENSE.txt as this is provided from drupal.org packaging.
  2. Files should have @file docblock. Also, format function docblocks correctly and inline comments should be full sentences. Make sure you are following coding standards. Overall you do a good job, but there are some small things. Also, not that coding standards are more than just PHP files.
  3. Your return for the AJAX call should be in JSON format so that code can do more with it. See http://api.drupal.org/api/function/drupal_json/6
  4. Your error descriptions need to be run through t().

Overall, this may be the best first module app that I have seen (I am still pretty new to reviewing apps). Just a few things.

--
Note: Please be patient with the CVS application process. It is all done by volunteers. Our goal is not to be arbitrarily slow or meticulous. Our goal is to get you CVS access and ensure that you are and will become a more responsible Drupal contributor. For a quick reference on what I look for in a code review, please see this article.

wiifm’s picture

wiifm
Primary language English
Location Wellington, NZ
commented
StatusFileSize
new statistics_ajax-2010.05.30.tar_.gz2.89 KB

@zzolo, thanks for the feedback,

Latest changes to the module:

  • removed license.txt
  • included @file docblock on all PHP files
  • converted all double quotes to single quotes
  • response is now completed by drupal_json() - thanks for the tip
  • t() is being used now
  • misc code stlye cleanups, e.g. 80 char line limit

Thanks for all your time on this

Cheers
Sean

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs work » Needs review

Remember to change status, when you upload new code.

zzolo’s picture

zzolo commented
Status: Needs review » Needs work

@wiifm69, thanks for the new code.

  1.    drupal_json(array(
      t('status') => t('error'), 
      t('data') => t('unknown HTTP method'),
    ));

    There is no need to translate the indexes of this array (and similar ones). Also, make error message complete, meaningful sentences.

  2. /**
* Valid permissions for this module
* 
* @return array an array of valid permissions for the statistics_ajax module
*/

    Should be the usual documentation for hook implementations.

  3. Utilize drupal_write_record, instead of your method of updating, then checking.
  4. Look at utilizing a token system to provide more robust protection against abuse http://api.drupal.org/api/function/drupal_get_token/6 This is just a suggestion and will not stop the application process.

Real close. The only real important one is the first one. Translating indexes can cause huge problems.

--
Note: Please be patient with the CVS application process. It is all done by volunteers. Our goal is not to be arbitrarily slow or meticulous. Our goal is to get you CVS access and ensure that you are and will become a more responsible Drupal contributor. For a quick reference on what I look for in a code review, please see this article.

wiifm’s picture

wiifm
Primary language English
Location Wellington, NZ
commented
Status: Needs work » Needs review
StatusFileSize
new statistics_ajax-2010.06.02.tar_.gz2.77 KB

Hi @zzolo,

  1. done, nice tip
  2. done, thanks for the pointer here, this makes complete sense
  3. I have done it this way, as this is the way core statistics works, I was only after replicating that exact db update. If there is a benefit to using drupal_write_record() then I will look into this
  4. tokens are not essential for my application, but you are right this may come in handy on larger sites, I will look to release this as an update in the future as time allows

Thanks for all your time in helping to make this module better

zzolo’s picture

zzolo commented
Status: Needs review » Fixed

@wiifm69, good work! I am approving your CVS account. Thank you for all our work and patience on this. I hope it was a good experience for you. The following is one last thing I noticed, but will not stop the approval.

  1. /**
 * Implementation of hook_settings().
 */

    There is no hook_settings(), though it would make sense if there was one for this very often case.

Please read the following resources to make sure you know how to use CVS and the specifics to the Drupal CVS infrastructure, as well as how to be a good module maintainer on Drupal.org. The Drupal community is very large and dynamic; we welcome you as a module maintainer and hope that embrace and challenge the Drupal community and continue to contribute.

--
Note: Please be patient with the CVS application process. It is all done by volunteers. Our goal is not to be arbitrarily slow or meticulous. Our goal is to get you CVS access and ensure that you are and will become a more responsible Drupal contributor. For a quick reference on what I look for in a code review, please see this article.

Status: Fixed » Closed (fixed)
Issue tags: -Module review

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

Issue tags: +Module review

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.