Comments

tatien’s picture

I had made a patch for this, I really thought I had posted it :(. Now I can't even find it on my computer.

I will get back to you ASAP. The patch was installed on a prod. website, I just need to get it back.

tatien’s picture

Status: Active » Needs review
StatusFileSize
new42.27 KB

Patched. Here is the patch for the .module file.

tatien’s picture

StatusFileSize
new702 bytes

... and here the patch for the .install file.

dww’s picture

Status: Needs review » Needs work

your patch appears to be against revision 1.107 of signup.module. many things have changed since then. ;) if you actually look at the patch, you'll see a huge # of lines of code that are modified, most of which are just undoing recent important changes to the module. please apply your changes to the latest version in HEAD (revision 1.116) and make a new patch that only includes your changes. thanks.

tatien’s picture

Status: Needs work » Needs review
StatusFileSize
new902 bytes

All right, I have re-implemented the patches on the 1.120 version. Here is the first part (install).

tatien’s picture

StatusFileSize
new12.69 KB

... and here the .module patch.

tatien’s picture

StatusFileSize
new12.16 KB

Sorry, I had forgot to remove some debugging code in the last .module patch. Please ignore it and use this one instead. The .install file was ok.

tatien’s picture

StatusFileSize
new12.16 KB

Again, some fixes to the patch. Please ignore the others.

dww’s picture

Status: Needs review » Needs work

A) 4 out of 20 hunks FAILED -- saving rejects to file signup.module.rej.

B) Code style: always use {} after your if () clauses, even if it's just a single line.

C) This comment doesn't make much sense:
// Set default type access to default general access if nothing was specified for this content type yet.

D) Code style: 'signup_access_' . $type -- no space between the . and string literals.

E) In this code block, if $node isn't set, trying to assign something to $node->type is an error. Also, see point (B) above:

+    if (!$node || !$node->type)
+      $node->type = $form['type']['#value'];

F) Code style: always include a comma at the end of the last thing you declare in an array, for example:

             $form_values['signup_close_signup_limit'],
+            $form_values['signup_access']
           );

should be:

             $form_values['signup_close_signup_limit'],
+            $form_values['signup_access'],
           );

G) Can you explain why the patch is doing the following?

-  $form['node_defaults']['_signup_admin_form'] = _signup_admin_form($node);
+  $form['node_defaults']['_signup_admin_form'] = _signup_admin_form(NULL);

H) Seems like this access stuff should be exported to views somehow. Like, you want to filter signup nodes based on if the current user is allowed to signup for them, etc.

I) signup_handler_filter_role() seems like duplicate code with core's user_roles() function.

dww’s picture

p.s. that said, this seems like it could be a useful feature when it is done. thanks for the patches so far! ;)

dww’s picture

Category: task » feature
tatien’s picture

Hi, I hereby address each of your points.

A) The CVS version probably changed since the time I submitted the patch.

B) OK.

C) Ok, I removed it.

D) Ok.

E) Ok.

F) Ok.

G) Because we need default options in that case (and NULL as the param of _signup_admin_form() returns default values).

H) Yes, that would be great!

I) Allright, I will change it.

Now, the problem is that I don't want to redo the work too often, so we'd need to coordinate a bit cause if I make a patch today for the current CVS checkout and we wait too long, it won't be valid anymore and we'll still have the (A) problem. So, I don't know what is the best way to do it, what do you think?

Thanks.

Totakeke’s picture

Has there been any updates on this feature request? It's something that would be very useful but it seems progress has been stopped dead in its tracks.

dww’s picture

Assigned: robeano » Unassigned

Sadly, I can't promise I always have time to review a new patch as soon as it's rolled. Problem (A) is basically just a fact of life in Drupalandia, sorry. :( You can watch the issue queue to see if there's something else actively being rerolled that might conflict. Now that this is running on g.d.o again, I might have to make some changes based on feedback from that experience, but I don't personally have a ton of time to work on signup for the next week or so, anyway. That said, the problems usually aren't too major and re-rolling is often a quick and painless task.

@tatien: All I can say is that if you roll a new patch that addresses all known concerns, I'll review it and either explain why it needs work or commit it as soon as I can. Feel free to assign this to yourself if you're going to actively work on the patch.

tatien’s picture

StatusFileSize
new13.76 KB

Thanks. Here is a new patch on the CVS.

juliekj’s picture

Is there anything on this patch for the 6.x release?

Apollo610’s picture

A D6 version would be fantastic... this sounds an awful lot like the Signups by Role module that's out there (also only in D5 form right now). But it would be great if Signups had its own permissioning built in.

Though that maybe a bit difficult considering the number (and scope) of outstanding requests there are out there for this high-demand mod.

duaelfr’s picture

Status: Needs work » Closed (won't fix)

This version of Signup is not supported anymore. The issue is closed for this reason.
Please upgrade to a supported version and feel free to reopen the issue on the new version if applicable.

This issue has been automagically closed by a script.

simon georges’s picture

Status: Closed (won't fix) » Needs work

Reverting recent closing.