[D6/D7] Delayed Ownership

Welcome,

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

Is the D7 code also available for testing purposes?

I would like to help making this an official project soon. Could you list me as a co-maintainer?

Hi,

here are the responses to your manual review done by me:-

.module file.

1) Remove $Id$. It is no longer required.
2) Remove commented code this should not be here.
3) @file doc comment missing though details are there. I believe @file should also be there.
4) Proper formatting of the code has not been done. Please refer this.
5) Global variables like $user etc. should be mentioned at the top. That is good practice.
6) true or false should always be caps. Though it dont makes a difference, but thats the standard.
7) Rather than drupal_goto in nodeapi you should be using $_REQUEST['destination']
8) HTML content should not be generated in the .module file. That work should be done in .tpl file. So you should remove this.

.install file.
1) Remove $Id$. It is no longer required.
2) Delete variables used in your module after your module is uninstalled.

.info file.
1) Remove $Id$. It is no longer required.

All these issues seem to have been already fixed in the D6 branch from http://drupal.org/project/1800776/git-instructions

What has to be done to move things forward?

After fixing issues, if any you should mark the status to needs review again.
Thanks
Parwan

A Drupal 7 branch is now also available via Git at http://drupal.org/node/1800776/git-instructions/7.x-1.x

This sounds really cool... and reading the code it seems well thought out and written. Cool functionality!

Curisoity question: On line 142 you make the session ID available as a form element. Does this present any potential issues for some one being able to get your session ID and jack it? Just something I was randomly thinking about...

One thought on wording:
Line 397 -- '#title' => t('Allow delayed ownership on'),

Maybe consider a different wording there such as 'Enable delayed Ownership'. The words "allow" and "on" are redundant in that situation.

Choice of wording: You decided to call the anonymous node form followup page a feedback page. Not sure if this is a Drupal term or your own, but it doesn't really seem feedback related since the user isn't really being asked to submit anything to you about quality. "Anonymous Follow Up Page" is what I'd go with. Not that it matters, especialy if it's a Drupal term :)

Something Positive: I just reviewed two modules prior to this. Both of them had almost non-existant project pages and their readme files were similarly devoid of content. Yours is much better! Congrats! Maybe consider pulling some of your stuff from readme into the project page as well though?

Hi,

Great job so far,
I don't know if Drupal 7 version should be reviewed in this Project application issue, but anyway. I really like the idea and want to contribute to review:

Review of 7.x-1.x branch:

1. Line 6: Replace anymous with anonymous.
2. Line 48: You don't have to specify 'access callback' => 'user_access' as this is the default callback. It can be removed.
3. Line 196: Maybe it would be better to use #markup for $form['links'] element instead of #value.
4. Line 229: You can use $form_state['redirect'] to redirect forms instead of drupal_goto. It will allow other users to alter your form and specify their own redirect of they have to.
5. In delayed_ownership_feedback_page() looks like a theme function with a template or preprocess will be better for HTML output. The $output .= 'something' in your case is not readable and hard to maintain.

Also please take a look at review script issues here:
http://ventral.org/pareview/httpgitdrupalorgsandboxorganicwire1800776git

I'm not changing status to needs work since the issue contains (D6) in title and reviewed branch is D7. Correct me if i'm wrong.

Good job!

Thanks to both of you for the feedback!

@ jnicola (#9)
The form element code is a remnant from the D6 code. In fact it isn't used anymore in the D7 version and will be removed in the next round of refactoring :-)

@ asgorobets (#10)
Thanks for your helpful suggestions! We hope to soon get the full project status so we can open a proper D7 issue.

JFTR

There's an interesting recent article on "Registration Through Site Engagement".

@organicwire

The session id in the hidden form field code can be removed from the Drupal 7 version as it isn't used any more.

Thanks!

I found some issues at http://ventral.org/pareview/httpgitdrupalorgsandboxorganicwire1800776git...

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

I'm a robot and this is an automated message from Project Applications Scraper.

6.x branch review:

• Remove delayed_ownership_update_6000() - it won't be needed once this is a full project.
• In delayed_ownership_form_alter(), instead of '#type' => 'hidden', which actually renders a form element for the user, use type value, which won't show anything to the user, but will still be available on validation/submission. Also, you can move the condition ($user->uid == 0) above the foreach() block - then you will only check the form ids if the user is anonymous. If the user is not anonymous you don't have to do any work.
• In delayed_ownership_user_pass_reset_submit(), should that be time() + (60 * $minutes) ?
• In delayed_ownership_nodeapi(), could variable_get('delayed_ownership_nodetype_' . $node->type, TRUE) default to FALSE instead? That would be a little safer. In fact, couldn't this code for $op = 'insert' all be moved over to _delayed_ownership_node_form_submit()? You've already done the work to make sure your custom submit function gets called when a anon user submits a node of the right type, no need to check all nodes again.
• In delayed_ownership_block(), use english to title your block, you can provide a translation package for german users, sorry! Also for rendering your block content, use a theme function. Right now no one could easily change the content of that block. Instead of creating your own ul/li tags, use theme('item_list') to do it for you.
• In delayed_ownership_feedback_page() - this all needs to go into a custom theme function. Avoid outputting bare html.
• Remove _delayed_ownership_defaults() - these are better off as additional variables_get('somevariable, [default]) when you can them. It does cause a little more code repetition, but it's much easier to override. Also use t() for all your translatable strings.

7.x branch review:

• Remove .gitignore.
• A 7 version is no longer a TODO in the README.
• I'm not sure about calling drupal_save_session(TRUE); like that - if the sessions arent being saved there's probably a good reason for it. I think Drupal's default is to always save sessions. Can you explain a bit more?

  // We have to save something in the session to activate it.
  $_SESSION['delayed_ownership'] = 'initialize_session';
 

There's an inherent security problem - using session id's + IP address is probably the best method you can use, but that info is all passed via plaintext and if intercepted could allow a user to have someone else's nodes assigned to themselves. I don't think it's a major issue, but if you could add something to the project page to let users know what the implications are, that would help.

----
Top Shelf Modules - Enterprise modules from the community for the community.

@kscheirer
Thanks for your thorough review.

Actually, I went through a lot of session related documentation while upgrading the module. Session handling with regard to anonymous users is a bit tricky. If I remember correctly, the following comment from the API documentation for drupal_session_inititalize() was the reason for using drupal_session_start():

// If a session cookie exists, initialize the session. Otherwise the
// session is only started on demand in drupal_session_commit(), making
// anonymous users not use a session cookie unless something is stored in
// $_SESSION. This allows HTTP proxies to cache anonymous pageviews.

drupal_session_initialize(): "Initializes the session handler, starting a session if needed."

Here are some more relevant session functions:

• drupal_save_session(): "Determines whether to save session data of the current request."
• drupal_session_start(): "Forcefully starts a session, preserving already set session data."
• drupal_session_commit(): "Commits the current session, if necessary. If an anonymous user already have an empty session, destroy it."
• drupal_session_regenerate(): "Called when an anonymous user becomes authenticated or vice-versa."

HTH
Frank

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.

I have the D7 version running on a proposal website (not public yet) where it works as expected.

The only minor issue I had found #2084719: D7: Change path of configuration page to admin/config/content (instead of admin/config) has been fixed.

There are still a few style issues reported here: http://pareview.sh/pareview/httpgitdrupalorgsandboxorganicwire1800776git. You've addressed all my blocking issues though, so this is RTBC from me. It's not required to remove .gitignore, just a general practice.

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

No further action is required, but the best thing you can do is get a Review Bonus by reviewing other applications. That will get you to the top of the list of projects to get reviewed (and hopefully approved). Only manual reviews count, just using http://pareview.sh is not enough.

Or if you wait a month and no one else raises any issues, I will send this application to "fixed" myself, which grants you "git vetted user" status and the ability to promote your sandboxes to full projects (including this one).

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

Changed headline level, updated the information on D7 version.

This needs work because of #2130351: Comply with coding standards.

Minor coding standard errors are not application blockers, any other major problems you found in your manual review?

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, organicwire!

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.