Personal Notes

The sandbox is at http://drupal.org/sandbox/marty/1147368. Complete with commits and a once over by berdir which I then fixed 2 weeks ago and committed.

Thanks for the feedback, finally made the time to go back and update the project page, which btw is at http://drupal.org/sandbox/marty/1147368.

Can someone please tell me what's next? There was a code review done at http://drupal.org/node/1148546 and closed on May 25, 2011. What else do I need to do for my project to get listed on D.O?

Marty

If you look at http://drupal.org/node/1148546 you'll see where Berdir did a code review and I responded and then it was closed at http://drupal.org/node/1148546#comment-4514326.

Hi, got through it all and just committed the changes.

- I'd move the line break outside of the translation function on line 192 of the .module file.

**this makes sense; I did a quick review myself to be sure this wasn't happening elsewhere in the code

- Your 'number of items' parameter in the following code is redundant ... if you're going to pass an argument
(or define a default), you probably shouldn't hardcode a value inside the function:
function _personal_notes_fetch_content($num_items=3)

**yes, had to look twice myself - it seems I didn't need or use this parameter at all here so I removed it from
the function and also from line 43 the one place it was being called:

$blocks['content'] = _personal_notes_fetch_content(10);
to
$blocks['content'] = _personal_notes_fetch_content();

and the function you quoted above to

/**
* Retrieve information from the personal_notes_notes table.
*
* This retrieves the notes made by the requesting
* user and then displays them on a block.
*
* @return
* String containing the notes.
*/
function _personal_notes_fetch_content() {
$results = _personal_notes_fetch_content_db(99);
while ($fields = db_fetch_array($results)) {
$out .= theme('personal_notes', $fields[title], $fields[note]);
}
return $out;
}

- Are you using any functions which mandate a minimum php version? If not, the php line can be removed
from your .info file.

**I've removed it since I don't know that I require any particular version of php

- As a matter of personal preference, I'd also prefer to see the sanitization performed inside the module
itself, versus in the .tpl template file ... this approach helps prevent themers from opening security
holes when overriding templates:
$out .= theme('personal_notes', check_plain($fields[title]), check_plain($fields[note]));

**Makes sense, I've added the check_plain calls to the module as you showed and removed them from the template file

The next two are simply suggestions, not 'blockers':

**And good ones, I implemented them both.

- As a useability enhancement (though not strictly necessary for approval), I wouldn't mide seeing a block setting configuration which lets users set how many items they want to display in their block, rather than hardcoded values.

**I did this globally ie. set by the admin for all users. Can you point me to a way to make this setting individualized so each registered site member that cares to set it for themself? That sounds like it'd be a nice feature to offer. Note this also changed the code above to

function _personal_notes_fetch_content() {
$limitnum = variable_get("personal_notes_maxdisp", 3);
$results = _personal_notes_fetch_content_db($limitnum);
while ($fields = db_fetch_array($results)) {
$out .= theme('personal_notes', check_plain($fields[title]), check_plain($fields[note]));
}
return $out;
}

- For robustness, I'd recommend using a dedicated index column on your personal notes database, instead of keying on the 'created' column ... the odds of having two notes created at the same time are almost nil, but it isn't technically impossible.

**Yep, I've added a serial field - notenum - and made it the primary key. Have left the created field, mostly in
case it should be handy in the future but am now simply using this field instead.

Marty

Sorry about that, did the git add and commit. Kind of new to it as you can see. Did the push now. Also verified it on d.o under 'my commits'. Next time I'll know to check that.

Thanks, more good advice. I've changed over to pager_query and committed.

Great, so is there anything more for me to do to get approved for a project page for this module?