The Node Tasklist module is used for creating checklists and interactive forms that are built with Drupal's content types. It fulfills a similar function to the "SEO Checklist" module but can be configured to manage any kind of task list that you can create using Drupal's content types.

The module creates a block and a URL for each enabled content type to display the "edit" form for the most recent node of that type. It sets a redirect on the submit function to always return you back to that page when complete. Nodes to display in the editor page/block are automatically generated by cron on a schedule which is configurable on the settings page.

This module is ideal for mobile apps where you need to continuously input data and be returned to the "edit" form of a node. If you use the block to edit the form, like with the page-based editor, clicking save will return you to the page of origin so you can continue your work from there.

This module is called "Node Tasklist" because it is meant to address situations where you would otherwise use paper to log your progress on mundane tasks but it can do so much more. It has been developed on my drupal.org Git sandbox.

http://drupal.org/sandbox/weal/1074540

Note: the module hides the "title" field (that will probably be an on/off option in future releases).

This module was made ready for production at Montréal's Blitzweekend at . An example of a content type for it is provided here: http://weal.ca/tl/blitz

I have run the code through the coder module and it passes all tests that I have run. I have tried to catch all errors gracefully. Please let me know if there are any additional steps that must be done to publish this project. Many thanks!

Comments

kiamlaluno’s picture

Status: Active » Needs review
Ryan Weal’s picture

I have posted a number of updates to this project since the initial application to make things easier for new users (and testers!). The module will now prompt you to update your configuration if you do not finish the setup process, gracefully catches errors, such as when cron has not been run yet, and has options for using external node scheduling and display.

Additionally, a bug that forced you to reconfigure blocks after adding or removing content types is no longer present. This means users can now roll this out to production websites without fear that incorrect content will be placed on the site as they add or update which content types are enabled.

The settings page now includes detailed documentation on how this module interacts with the user timezone and site timezone settings. Future versions will contain more time configuration options for node creation.

svendecabooter’s picture

Status: Needs review » Needs work

Hi,

I have reviewed your module, and found a few bugs with it. There are also some minor coding improvements.
I have added all of that to the sandbox issue queue.

Once you have addressed the bugs, this module could become a full project as far as i'm concerned

Keep up the good work
Sven

Ryan Weal’s picture

Status: Needs work » Needs review

Thanks for the detailed review. I have worked my way through all of the items in the issue queue as of yesterday.

Ryan Weal’s picture

Component: new project application » module

Are there any additional items that I should take care of to get this project application approved? A D7 version will come soon.

Thanks again for your time.

jordojuice’s picture

Priority: Normal » Critical

Setting priority according to priority guidelines so we can get this review continued.

davidhernandez’s picture

Priority: Critical » Normal

Looking over the files I see a few minor things; remove the $Id lines from the files, use the long style php tags (<?php not <?), and some of the functions are missing leading comments. You have them most places, but a few are missing. http://drupal.org/node/1354

There are a few inconsistencies in your spacing. This is really not that important, I'm just pointing it out for completeness. You may not notice it in your editor, but it shows in the repository viewer.

Also, I'm not sure adding personal copyright in the module is necessary, or appropriate. I'll have to find documentation on that, or get a second opinion.

Ryan Weal’s picture

Hello davidhernandez,

Thanks for the review.

1. Id tags have been removed from the info and install files, sorry I missed that. As noted above, I only added these to comply with the coder module's recommendations earlier in the process.

2. I have not seen any documentation on drupal.org suggesting <? over <?php anywhere and grepping my modules folder I see that is everywhere in drupal code. I believe it is also the recommended practice when doing php blocks, etc, to put the tag in any PHP input filter textareas as noted in the #description of those fields. Can you point out where that rule is?

3. Spacing. I'm not sure specifically where you are talking about here (no link or line numbers provided) but I removed any instances of three line breaks in a row. Hopefully that helps?

4. A personal copyright is not only my legal obligation here in Canada to declare myself as an original author but also the common practice with contrib modules. Running a quick survey through my contrib modules on my site I found 633 copyright notices, most of which have been attributed to the Free Software Foundation, and 71 of which are attributed to the original author. The license is clearly a GNU public license so I again would like more detail on why you think this method is or isn't appropriate.

Sorry if this response sounds grumpy. I submitted this application 3.5 months ago and I have been following the discussion on why the process queue is taking so long. I'm ok with things taking awhile... but...

I work with drupal modules a lot and I see some very questionable code published on drupal.org every day and yet I still want to be in compliance with all the standards... even though some (ahem, many) modules already published on drupal.org do not abide by the coding standards at all. Having said that, I hope you can appreciate that now waiting even longer, just to remove spaces and codes that the project application itself introduced to my code seems excessive. I will, of course, try to improve the commenting as noted above.

I hope I'm not burning any bridges by saying this stuff. At this point, I would rather focus my time on getting the Drupal7 build of this module out the door so I can start tending to my other coding projects again.

Thank you again for your notes and pointing out the documentation standards page. I've read it over and I'll definitely be revisiting it to best align my coding practices with the gold standard here.

sreynen’s picture

Status: Needs review » Needs work

davidhernandez, make sure to set the status to "needs work" when you see issues.

Ryan Weal, on your numbered list:

1. looks good.
2. I think you misread what davidhernandez wrote. Long tags are preferred, since they work on all PHP installs. You have short tags in a few files node_tasklist.install. That won't work on some servers. I think coder.module should tell you that.
3. I don't notice any problems with spaces in the current code. In any case, as davidhernandez said, that's a pretty minor issue.
4. The Drupal.org packaging system automatically adds a LICENSE.txt file to packaged downloads, so we can keep that consistent across everything on Drupal.org, which all shares the same license. For more on why we don't add licenses to specific projects, see http://drupal.org/licensing/faq

You're certainly not the first person to be annoying that this process takes so long. Many of us are doing reviews now because we were similarly annoyed. I'd encourage you to get involved in making it better. Just keep in mind that the people doing these reviews are applying policies of the larger Drupal community, not making up those policies. If you disagree on the licensing policy, for example, the legal group is a better place to discussing changing it.

Ryan Weal’s picture

Regarding #4 I'm not against the licensing at all, I just want to be sure I'm following the rules. Thanks for the link to the guidelines.

ps. I guess I can be quite annoying. ;)

davidhernandez’s picture

Status: Needs work » Needs review

2) To clarify, since you wrote it opposite of what I stated. "<?php" over "<?" Common practice. From php.ini:
; NOTE: Using short tags should be avoided when developing applications or
; libraries that are meant for redistribution, or deployment on PHP
; servers which are not under your control, because short tags may not
; be supported on the target server. For portable, redistributable code,
; be sure not to use short tags.

I haven't seen short tags (<?) used anywhere in Drupal, but I'll take your word for it that you saw them. I mostly pointed it out because you yourself are not consistent with them. You have them in one file, but not another. I'm just trying to make you aware of something you might have missed.

3) Random places, but again, not important.

4) I can't comment.

Just to reiterate, this are only minor things. I am not suggesting they would hold up your approval, but please do not use the "...I see some very questionable code published on drupal.org every day" reason. There are a lot of bad things that go on here, and we are trying to limit them, and make people aware of them, not contribute them. The point in bringing up little things is so people can avoid them in the future. It makes all of us better contributors.

That said, I'm still looking over your module. I only commented on some code style things because they were the low hanging fruit I noticed. It will take some time before I can say anything else. I'm sorry you had to wait this long, but I did not start your review. I can only do what I can now. I'll try to look over everything as soon as I can to move things forward.

Bridges are rarely ever burnt with me.

davidhernandez’s picture

Just saw above. I'll have to type faster next time. :-)

jordojuice’s picture

Yes, long form opening tags are part of the coding standards and common format. I haven't personally seen them in Drupal core or contrib that I can remember. See http://drupal.org/coding-standards#phptags

Always use to delimit PHP code, not the shorthand, <? ?>. This is required for Drupal compliance and is also the most portable way to include PHP code on differing operating systems and set-ups.

Indeed, this is why the filter will not even interpret <? as code, as you see. And required is a pretty strong word!

Ryan Weal’s picture

Well, I have learned a few things today. Points #2 and #4 are now resolved. I had misread the statement on the php tags and now that I see what we're trying to achieve it makes way more sense. I too was favouring the Unix-shebang-style file intro, but not doing it everywhere, so sorry for the mixup there.

Thanks again everyone for the reviews.

As an aside, this did prompt me to finally start *asking* some questions on IRC which is somewhat new to me (at least in the Drupal community). I have always been in the habit of offering random help in the #drupal-support channel and then leaving my client logged-in for days. I finally found an IRC client that works for me so I'll be more active in the channels when I'm online because I'll actually BE online.

davidhernandez’s picture

Status: Needs review » Needs work

Looking over the code more thoroughly:

Regarding my previous spacing comment, just look over the code using the repository viewer. That is how I see it both online, and when downloaded. I saw what looked like some inconsistency, with a double blank line at 186-187 of the .module file, and a seemingly random blank line in the CSS file. Also, a lot of trailing white space throughout the files, and a lot of blank lines in your cron function. I don't really care if you change them, I just wanted to make sure you were aware of it, in case it was unintentional. Some times editors show things differently. That is all.

Because you do have a CSS file, I want to make you aware that there is a CSS coding standard, as well. http://drupal.org/node/302199 . Pay particular attention to the right-to-left component, since you are doing some floating. Otherwise, not terribly exciting, but you should probably at least comment the file. (Yes, I know I'm a bit pedantic when it comes to commenting. Somebody's gotta do it.)

Line 233: I noticed a double-space between the two sentences in your watchdog message. Make sure any translatable text is exactly as you intend, because typos can annoy translators.
Line 254: Remove the space after "Tasklist: ". Put it outside the t() function.

Don't use t() in watchdog messages. http://drupal.org/node/323101 (towards the bottom)

Include a description in hook_menu for your admin/settings/node_tasklist item. Because you don't have one, no description shows up on the admin page.

I don't believe it is required, but it would be a good idea to include a hook_help function. Everyone that works on help initiatives will like you very much if you do.

On to actual functionality:

I installed on a clean install of 6.22. Everything looks to be working fine. This is actually a pretty nice idea.

Starting on line 58 you are doing this:

$action = $_GET['q'];
$url_parts = explode('?', $action);
$url_parts = explode('/', $url_parts[0]);
if ($url_parts[1] == 'node' || $url_parts[2] == 'node') {
if ($url_parts[3] == 'edit' || $url_parts[4] == 'edit') {

Why not use arg()?

sreynen’s picture

Just to be clear, I meant "annoyed," not "annoying." :p

Ryan Weal’s picture

sreynen: I enjoyed the double entendre, it made me laugh! I try not to take myself too seriously.

davidhernandez: good call on arg(), I should be using that in this case. I'll work through all the items in your post in my next update.

Thanks

Ryan Weal’s picture

Status: Needs work » Needs review

All changes requested in comment #15 have been taken care of. I included a hook_help function which has some additional hints for site administrators. Great tips! Thanks again.

davidhernandez’s picture

Status: Needs review » Needs work

Coder has complained about a few things. Mostly minor things, like SQL keywords. They aren't important to code functionally, but do improve readability. Consider making changes, though this is not something I would delay your approval over.

Coder also alerted to this:

Line 288: Potential problem: drupal_set_message() only accepts filtered text, be sure all !placeholders for $variables in t() are fully sanitized using check_plain(), filter_xss() or similar. (Drupal Docs)
drupal_set_message(t('No matching content for %type. Please !create_link manually.', array('%type' => $type, '!create_link' => l(t('create one'), 'node/add/'. $type))));

Since you are passing the machine name for the type, which is not normal user submitted data, this probably isn't a concern. You might want to double-check on your own.

You have a double space between sentences in your hook_help function. The standard is single spacing. I mostly mention it because of translation.

Lastly, I noticed a minor XSS vulnerability in the admin settings form. The content type names returned by node_get_types are not sanitized. You need to pass them through an appropriate function; usually check_plain.

Other than that, looks good. Fix the XSS issue and I'll RTBC it.

Ryan Weal’s picture

Status: Needs work » Needs review

Hello again,

1. SQL keywords have all been converted to uppercase. I noted that there is a setting in coder to show the minor updates so I guess I was not seeing this earlier. Running that as my default now.

2. Sanitization of $type variable. I could not replicate this warning (it is possible I fixed it before I ran coder though). I followed this variable up the chain and put a sanitization on the arg() that is passed from hook_menu so that the protection applies to everything from that point onward. It *is* a machine name so it is probably of low concern but it does not hurt to do one extra check. In this case hook_menu is the only origin of user input. Good find, thanks.

3. hook_help spaces have been corrected. Force of habit on that one.

4. XSS issue with node_get_types('names'). Another good find. Upon checking the API documentation this makes a lot of sense. I used the code recommendation listed on the API page to resolve this issue.

That looks like everything. I've posted the most recent build and I get zero errors in coder. Thanks for the reviews!

davidhernandez’s picture

You missed the two node_get_types calls that are part of the fieldset form element. One in the hook_block function.

I noticed a weird thing. When I add one of the tasklist blocks to the left sidebar in Garland, the node/add/{type} and t/{type} pages loose the body textarea. It gains 'style="display:none"'. Not sure why that is happening. It does not happen in the right sidebar. I'm not seeing anything obvious that would cause that.

Also, I noticed that on the node/add/{type} the title field goes away. You are autofilling titles, but is that the case everywhere? It seems to completely override the content type. I'm just making sure I understand the functionality.

Coder for me still reports the drupal_set_message warning, but I see that the content is sanitized before it gets there. Since everything seems to originate in node_tasklist_get_name you may want to just sanitize it there, and then you'll know it is done everywhere it is sent. I'll leave that for you to ponder.

Otherwise, there are no other coder warnings, even minor style ones. Everything looks good. Correct those last node_get_types calls and I think we're done.

davidhernandez’s picture

Status: Needs review » Needs work
Ryan Weal’s picture

Status: Needs work » Needs review

Hey,

The other calls to node_get_types have been updated.

Regarding the body field, I cannot replicate this issue. I'm using zen templates and I switched to Garland but I could not reproduce.

The title field is "taken over" by this module. I noted it in the OP (look way way up this thread) that this is something that I will make optional. Earlier versions did not have the option to display the 'most recent' posting so it wasn't an important feature in early builds. I'm going to create a new issue for that... not sure if the sandbox queue carries over but I want to make note of it somewhere. At the moment I'm mid-way through the Drupal7 build so I am probably going to proceed with that as my next major functionality update. In the meantime I'll make a note on the project page so that people know why the field is "missing".

I also cannot find that coder message you speak of. I'm running coder 6.x-2.0-beta1 and I have enabled 'minor' bugs to display. Is there another setting I should enable for deeper analysis?

Thanks again!

jordojuice’s picture

> not sure if the sandbox queue carries over

Indeed it does.

davidhernandez’s picture

Status: Needs review » Reviewed & tested by the community

I think I'm running the latest dev of coder. Maybe that is the difference.

The changes have been made, and everything seems to be working fine, so it should be good to go. I think I've beaten you up enough already.

I know the process has taken a long time for you, but hopefully we are all better off for it. Good luck, and keep contributing!

Ryan Weal’s picture

> I think I've beaten you up enough already.

Rofl!

I have learned a lot (much more than I was expecting!) and I'm glad I went through the process. Thanks!

sreynen’s picture

Status: Reviewed & tested by the community » Fixed

Ryan Weal,

Thanks for your contribution and welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

Thanks, also, for your patience with the review process and for participating in the code review group as we work to improve this process. Please consider helping out as you can to review other projects that are pending review.

Ryan Weal’s picture

Thanks. Just promoted it:

http://drupal.org/project/node_tasklist

I'll drop in on the project applications and try to add my feedback when I have the time. As part of my learning in this process I have also discovered the qa.drupal.org project so I hope to contribute to that too.

Status: Fixed » Closed (fixed)

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