I am just starting to try out the signup module and it sounds like there are a lot of feature requests being proposed.. in case this one hasn't been mentioned (or it is available and i can't find it) i thought i would add it to the list:

- i think it would be handy to be able to define some sort of message on a per node basis that can be added to confirmation email.

- this would be defined by admin when setting up an "event" in the on the fly section for signup; this textarea field would then be assigned to something like %notification which could be added to the admin -> settings for the confirm email

Actually, on the project page it sort of suggests that features for confirm email, etc are "options per node". But i think it is just worded wrong since pretty sure this isnt available now???

Peter Lindstrom
www.LiquidCMS.ca

CommentFileSizeAuthor
#4 signup_4.patch13.87 KBliquidcms

Comments

liquidcms’s picture

liquidcms commented
Status: Active » Closed (fixed)

ok, found my problem

it is possible to customize email confirmation on a per node basis - but it is necessary first to go configure that node type to accept signups.

i think this is quite confusing since without doing this - signup settings still get added to the node/edit for that type - the only diff is that without enble for that node type only option in settings is "enable signups" - but when you enable for that type; many more options are available.

i think the BUG is that if not enabled for that nde type - nothing should be visible for signup settings when adding content - at tleast this way people would go looking to turn it on.

liquidcms’s picture

liquidcms commented
Title: custom email notices » signup workflow logic is incorrect
Version: 5.x-2.x-dev » 4.7.x-1.x-dev
Category: feature » bug
Priority: Normal » Critical
Status: Closed (fixed) » Active

ok, more testing and i think i need to re-open this

can someone (Derek??) let me know:
1. if this is what they get with signup module? AND
2. if there is a reason behind this logic (is this the expected behaviour)?

- if i go to admin->content types and enable signups for a particular content type then i now get the ability to define signup specifics when i add a node of that type
---- however it seems as though i now get signups enabled for every node of that type and there is no way to disable it (since when i edit the page the "enable signups" is no longer visible

HOWEVER

- if i don't enable signups for a node type then i do not have the ability to define signup info (like email confirmation text) for a node of that type
---- although i can now see (and the only thing i can see) checkbox to en/disable signups for that node

I think part of this may be that the variables and table entries do not get cleaned up when content type config is changed (for example i disabled for pages; but now i can no longer set disabled for a node of type page)

I wouldn't mind taking a crack at fixing this - i think it is not too complex; just logic mods for when things are active or not

BUT - i need to know the 2 answers above first (although i will likely change for myself anyway since i dont think it is usable as is) - but i wont submit a patch if people believe this is the expected logic.

Peter Lindstrom
LiquidCMS - Content Management Solution Experts

dww’s picture

dww
we/he/they
commented
Status: Active » Closed (duplicate)

yes, i agree the workflow and settings are broken.
see http://drupal.org/node/67509 and http://drupal.org/node/49007

liquidcms’s picture

liquidcms commented
Title: signup workflow logic is incorrect » fixed some of the signup issues
Assigned: Unassigned » liquidcms
Status: Closed (duplicate) » Fixed
StatusFileSize
new signup_4.patch13.87 KB

it isn't whining if you fix it...

so here is my first shot at fixing some of the issues I saw when trying to use the signup module.

also, this is the first patch I have ever done - so if it isn't right; then why not get the full module code from here: www.liquidcms.ca/signup and make a proper patch and post it here :)

this patch is against the 4.7 latest rev posted on the project page.

a description of what I have done:

- added a new field to signup table (signup_enable)

- admin/settings/signup is untouched
--- this sets up defaults for signup info that can be set for individual nodes

- you need to enable signups on a content type basis (e.g. admin/settings/content-types/page and set "enable signups"
--- if this isn’t done then nothing related to signups shows up when you create content of that type

- when you create a new node of type that is enabled for signup the signup section at the bottom will now have a copy of the signup settings info (confirm email, reminder email, where to send notice of signup, etc)
--- by default this will have values defined in settings
--- you can edit these as you like for this node
--- there is also a checkbox (only thing that was there before) to enable signups for that node
--- if signup enabled for that node when you save it you should see signup ability now for that node

I think, but haven’t tested, that if you create a node with signup and then disable for that node type - you will not be able to add signup to new nodes of that type; but it will still exist for previous nodes that were created when it was enabled (I didn’t do anything to make this happen; likely there before and seems like correct function

If you disable signups for a node; admin can still see signup info for signups that were added before signups were disabled for that node - but I don’t think user will see that they have signed up for the node - I guess this also seems correct since you have now disabled signups - don't think that is the same as when "closing" signups - in other words when closed user should still see they are signed up (and it think that is what happens)

I think that’s about it - I hacked a lot of the code; so quite possibly I have busted something - if you test it out and find issues let me know.

- patch should also include changes to signup.install (for new added field)
- patch may include a change to signup.theme since I think I made "name" a required field

Peter Lindstrom
LiquidCMS - Content Management Solution Experts

dww’s picture

dww
we/he/they
commented
Title: fixed some of the signup issues » settings to enable/disable signups per node
Status: Fixed » Needs review

first of all, thanks for supplying a patch! however...

the "fixed" status is for when *i* commit the fixes to CVS. ;)

what you meant was "patch (code needs review)" which means: "here's a patch that fixes all my problems, please review it and commit it to the upstream source if appropriate, or set it to "patch (code needs work)" if there's anything wrong with it. ;)

also, as it says right under the title "Note: modifying this value will change the title of the entire issue, not your follow-up comment.". so, please only change the title to further refine/clarify the scope of the entire issue, not to give a title to your individual follow-up.

i'll take a look at this sometime today and let you know what i think...

thanks again,
-derek

liquidcms’s picture

liquidcms commented

as with most of my posts.. i fix becasue i need it. Feel free to commit as you see fit.

as for title of post - i knew i was changing it; but it was oringally my post - so didnt see the problem. - also, my patch fixes more than just what your title suggests - and i wanted to let people interested know there were fixes to a few things - easiest way was to title as i did (but i will leave the title as you set it - like i said the changes work for me and i am trying to let others take advantage; i took a shot - i feel pretty good about my helpnig out the community.. but in the end... i really just needed the code).

feel free to review, comment, commit, etc at your liesure.

peter...

dww’s picture

dww
we/he/they
commented

a) again, i'm happy you're trying to help.

b) i'm just trying to get you up to speed on how to be a helpful contributor, since you've already put yourself in the small minority of people willing/able to actually patch the things you care about. ;) given that, it seems like it's worth a little of my time to help you become an effective contributor, which will help the drupal community overall, and will help you (and others) avoid getting frustrated if there's a miscommunication about how things work with the issue queue, patches, etc.

c) you're not alone in your "i fix because i need it" point of view. most of us are like that. ;) however, what makes open source work (and what makes drupal great) is that people contribute what they fix so others can benefit, too. so, again, i'm glad you're helping, but for your help to really be valuable, we should get your fixes into the official code.

d) as for the title: you and i aren't the only people looking at this issue. other users of signup might care, so it's good to keep the issue title as complete and accurate as possible. if my quick guess of an appropriate title wasn't quite right, feel free to further refine it. but, setting it to "fixed stuff" (or equivalent) doesn't help anyone else who wants to try to test, review, or improve upon your patch (since they'd have no idea, without a lot of additional work, what the problem is that's being solved).

thanks,
-derek

dww’s picture

dww
we/he/they
commented
Status: Needs review » Needs work

finally had a chance to look at the diff itself (though i haven't had a chance to test it). over all, it seems like a fairly reasonable approach. i'll have to go over it more closely and test it well.

meanwhile, a few issues i noticed just from looking at the diff:

  1. a few of the lines in the patch just add stray whitespace, which we try to avoid.
  2. the part about setting "name" to be a required field belongs in a separate patch (for http://drupal.org/node/67067 -- in fact, there's already a patch there that just needs a little work and it'd be ready to commit).
  3. we need to add a signup_update_4() that modifies the schema for an existing installation of signup.module so that people can upgrade to your version without breaking their site.

let me know if any of that doesn't make any sense.

thanks!
-derek

liquidcms’s picture

liquidcms commented

read post for number 2 - makes sense; mostly i wasn't worried about the signed in user part since no one registers on my site.

again i would have to say i am a bit confused about #3 - from what i can tell the signup module currently doesnt fill in the db records for a node (although it does make the record) since there is no place to enter data - so basically everyones db is already busted - but likely i am missing something here.

or, are you suggesting taking default values (record 0) and filling those into the missing values for the signup records which have been added - that likely is a bad idea - since people may have done what i did since things like email confrim werent being filled in - i just went to db and entered content directly

i am not familliar with updates. is this part of the module? also, my guess would be that generally an update should not modify any data in existing fields - but adding new fields (or tables) is ok.

certainly an update to add the additional requried field is a good idea

i doubt i will do much more work on this until at least someone has tested it and let me know that it is working (or not) - after that i might be able to do the mods.

peter...

dww’s picture

dww
we/he/they
commented
Priority: Critical » Normal
Status: Needs work » Closed (duplicate)

ok, i still don't see the critical bug here. yes, the UI is confusing, but it's not like the whole module fails to function because of this. and, this is *still* duplicate with http://drupal.org/node/49007 so let's move any further effort and discussion back there.

thanks,
-derek