As I went through the module I came across a few issue.

  1. When anonymous user views the 'Claim' link it should be the user/register page, though it still is the claim link; hence claims by anonymous users are saved.
  2. After a claim is approved, users can still see the claim link, users you had previously applied for claiming the node still see 'Claim Pending'
  3. Saving the claim is triggered on registering a new user, where as it should be triggered when user has just logged in and then check if the $_SESSION variable has the key 'nodeownership' set
  4. There was no need of attaching the claim form with the register form since a site builder might already have similar fields in their register process. Besides the claim form can easily be shown after the user is logged in and then the claiming process will proceed as usual
CommentFileSizeAuthor
#5 nodeownership-multiple_fixes-1866532-5.patch5.3 KBrobcarr
#4 nodeownership-multiple_fixes-1866532-4.patch5.31 KBrobcarr
#2 nodeownership-multiple_fixes-1866532-1.patch6.07 KBSGhosh
#1 nodeownership-multiple_fixes-1866532-1.patch6.07 KBSGhosh

Comments

SGhosh’s picture

SGhosh commented
StatusFileSize
new nodeownership-multiple_fixes-1866532-1.patch6.07 KB

Including the patch in this comment.
Patch also contains -
Configuration settings from admin ui for - Claim and pending claim texts.

SGhosh’s picture

SGhosh commented
Status: Active » Needs review
StatusFileSize
new nodeownership-multiple_fixes-1866532-1.patch6.07 KB

Submitting patch for review. (Setting status to Needs Review)

robcarr’s picture

robcarr
Primary language English
Location Perthshire, Scotland
commented
Status: Needs review » Needs work

Patch fails:

/nodeownership/nodeownership-multiple_fixes-1866532-1_0.patch:21: trailing whitespace.
  
error: patch failed: nodeownership.module:337
error: nodeownership.module: patch does not apply

The version (7.x-1.3) I downloaded has the line (339)

drupal_goto('admin/config/nodeownership/nodeclaims');

Whereas the patch is trying to replace a line:

drupal_goto('admin/content/claims');

This module seems to have caused a lot of problems with the menu system. Lack of a DEV release and config control may also be a wider problem with this module too.

Will attempt a new patch later on.

robcarr’s picture

robcarr
Primary language English
Location Perthshire, Scotland
commented
Status: Needs work » Needs review
StatusFileSize
new nodeownership-multiple_fixes-1866532-4.patch5.31 KB

Updated patch attached.

Feeling less comfortable with the use of the View that comes as part of this module - it's not a particularly robust approach. If, for example, the site builder changes the path of the View (or clones the View for deeper customization) - all easy to do in the Views UI - then a lot of the drupal_goto calls will fail.

It would be better if this module came with a disabled View (for cloning etc), but also came with a hard coded table fixed to the path /admin/content/claims

robcarr’s picture

robcarr
Primary language English
Location Perthshire, Scotland
commented
StatusFileSize
new nodeownership-multiple_fixes-1866532-5.patch5.3 KB

My bad - disregard the last patch - URL typo.

SGhosh’s picture

SGhosh commented

I changed the redirect path to the list of claims, I suppose this should have been so in the first place and not redirect to the admin settings page, isn't it?

SGhosh’s picture

SGhosh commented
Issue summary: View changes

Added another issue

  • grasmash committed 8be9d8e on 7.x-1.x 
    Issue #1866532 by arrrgh, SGhosh: Fixed Patch for multiple fixes -...
grasmash’s picture

grasmash commented
Issue summary: View changes
Status: Needs review » Fixed

Most of this has been committed. However, I removed the check for claim status, as it prevents a node from being claimed more than once, which is a valid use case. Such a feature would need to be configurable.

Status: Fixed » Closed (fixed)

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