Terms of use is a very nice lightweight module that can solve legal requirements of many websites. Unfortunately the support is not keeping up, if not ceased for the last year. Many desired features are still missing.

I combined all the patches that address what I was trying to do for my websites, plus some of my own changes. Patches are still not committed, and applying each is quite challenging since they start to conflict:
#577966: Integration with checkbox_validate module.
#605392: Terms of Use versions
#354881: Multiple node links in terms checkbox label
#296440: Page is to long: use scrool bar to display Terms of Use
#299416: translations support
#719946: TOC Checkbox when admin creates an account - feature or bug?

Here's a condensed list of changes:
- Track version of Terms and each user's acceptance, force user to accept new version
- Parse more tokens in the checkbox label (@"Node title" in addition to @link)
- CSS injection to limit height of full text of Terms node. Height is configurable
- Add a preview to settings form of how "Tems" fieldset will look on user registration and acceptance forms
- Remove Terms from admin "add user" form. (Untested, but supposedly it will force the user to accept the terms upon first login)
- Checkbox label parser moved to theme_terms_of_use_checkbox_label() function
- Compatible with checkbox_validate, or fixes "required checkbox" bug on its own if checkbox_validate module is not installed
- Add multilingual variables support. Together with multilingual nodes provides full internationalization
- Fixed db_rewrite_sql() failing in multilingual sites (no bug filed yet?)
- Preserve selected node title / node id across AJAX switch between title/id on settings form

I hope the attached patch will be useful to others, and hope to see it makes into the CVS.

Comments

akolahi’s picture

thank you!!

gagarine’s picture

Nice... Try you patch next time I need TOU (but hop is review and committed before ;) )

iva2k’s picture

StatusFileSize
new40.54 KB

While testing patch in #0, I found few new issues coming from original patches inter-incompatibilities.

A new patch attached with the following fixes/features in addition to the previous:
- fixed logic that forces new version acceptance form - now it allows nodes linked from Terms node or checkbox token links to be shown (allowed nodes). Without it any links are pointless - logic from versions patch prevents opening any of the links from links patch.
- added hook to recalculate allowed nodes when involved nodes are edited/deleted, or translations added
- added logic to allow terms configuration without a node, can use checkbox link tokens. If neither is present, terms of use are disabled (inactive).
- admin user is not forced to accept new terms
- fieldset name changed to be a title, "use fieldset" checkbox added. Fieldset only applied to user registration form. New version acceptance form uses title, but not the fieldset
- Moved configuration form to .admin.inc file for better performance
- Various wordings improved

I SHOULD MENTION THAT THIS IS NOT A SIMPLE COMBINATION OF ORIGINAL PATCHES ANYMORE. The code is a significant refactoring of the whole Terms of Use module. I also should mention that I am not a maintainer and have no CVS access to this module, so don't expect any commits from me.

I know of only one remaining issue: - remove version variables from all user accounts upon uninstall.

Testers - please post here your test results.

miro_dietiker’s picture

To me the integrated link extraction and code with it looks way too complex for integration in a simple module like this. You might take this functionality from some external module... (or put it into some..)
What's actually the reason for this complex part in the patch?

gagarine’s picture

I think it's for "fixed logic that forces new version acceptance form - now it allows nodes linked from Terms node or checkbox token links to be shown (allowed nodes). Without it any links are pointless - logic from versions patch prevents opening any of the links from links patch."

But I agree _terms_of_use_prep_allowed_paths and _terms_of_use_extract_links sound really complicate. Perhaps remove this part and try first a commit?

Someone ask Caroline Schnapp about what they want to do with this module?

iva2k’s picture

@gagarine
"But I agree _terms_of_use_prep_allowed_paths and _terms_of_use_extract_links sound really complicate. Perhaps remove this part and try first a commit?"
You can just comment out the calls in question from #3, or try the patch in #0 - it has none of this logic. You will find that when you bump up the version of Terms, links don't really work, neither from checkbox label, nor from the node. I think it is a big problem in #0 as well as in all the original patches combined, and make all of them pointless. Thus the complicated logic in #3 that resolves it by extracting all the referenced links and allowing them to show without "accept" form interception. An alternative would be to let admin enter all node ids into a variable, but with translated nodes it would be a really difficult exercise.

Link extraction code came from the linkchecker module verbatim. I even left most of the unused code "intact" to facilitate expanding it (should someone want to have <video> or <src> etc. tags processed), comparison and backporting in the future (we can't make linkchecker a dependency).

Acknowledging the complexity, all new link extraction logic is moved to a separate admin.inc file that is loaded only when needed - when terms of use are updated. It also lightens up the original code by moving admin form out, making the main code faster.

Re: "Someone ask Caroline Schnapp about what they want to do with this module?"
I sent her a message via drupal.org a while back... No response. Last commit 1 year ago. I sense abandonment here.

iva2k’s picture

StatusFileSize
new43.18 KB

Updated patch attached. Please use the latest patch for testing / review.
- added simplistic views support (expose Terms of Use Version field for user views). No sorting or arguments support yet.
- added couple of special cases - print, printpdf paths (support print.module). Some users may want to print the Terms, or at least open them in a separate window.

iva2k’s picture

I ideally would want to hand off the code to maintainers, either old or new. Alternatives are:
1. Find old maintainer for terms_of_use (I tried to no avail)
2. Find new maintainer (any volunteers?) and take ownership of terms_of_use
3. Create new terms_of_use2 module (I will be a maintainer)

Simplest at the moment is 3, but more correct way is 2. Any opinions?

joostvdl’s picture

Go for option 2.... (which is almost the same as option 3 i think...)

kars-t’s picture

Status: Needs review » Needs work

Hi

I have CVS access to the module now and will commit any usefull patches.

There are some coding standard problems in this patch most are about spaces after line end. Please take a look at it.

The real problem about this patch is it's size and multiple features. This will be very difficult to test. I second miro_dietiker that a monster patch is not a good idea. But I don't want to loose your work :)

I will write a simpletest for this module during the next weeks so we can make sure it still runs.

About the single patches. Did you change their code or did you just add them? So maybe we can break this thing appart again and patch it step by step?

iva2k’s picture

@Kars-T
I purposefully did not do code cleanup, because original code fails many style checks, and patch will be incomprehensible. As a result I did not clean my code either.

I fully support simpletest approach... however, will it be only old functionality, or desired new functionality also?

Please read #6 - it should answer your question... To reiterate:
1. Not all addressed issues have patches
2. Incompatibility of pre-existing individual patches
3. Increased the combined functionality (You'll have to combine all '- ***' lines in my comments above for the full feature list)

I consider this patch to be more of a 2.x branch. The intermingling of the features is just too intense. I can't see how the patches can be split out without resolving commit order first. There are changes to individual parts as the code moves through the functionality changes. For example, multilingual patch is simple, but will be changed since I modified and added variable names for more functionality. Doing multilingual patch last makes sense, but it moves smaller functionality change behind bigger functionality change.

So the problem of splitting up is in deciding on the order in which you would want to apply separate patches. Without it I can't tackle the split. If you come up with a roadmap, I think I can help you with individual patches. I have only the concern of how long will it take to get through it. I can't stick around for few months for this to be done, and that's how long I think it will take with all the cycles of open-source review process.

I want you to first consider a branch approach (the one I would take if I became a maintainer):
1. create a new branch (version 2.x)
2. patch all code at once there. Do code cleanup afterwards.
3. release alpha for community tests, write simpletests
4. hash out remaining bugs if any
5. release when ready.
This way you don't even have to spend time on making sure 1.x version is not broken, splitting the patch up, figuring out what should come first, resolve dependencies, wait for people to respond to each cycle, etc. I think this would shorten the cycle dramatically, and make everybody's life easier.

What do you think? If you want to chat - reach me on Skype iva2k_ (IM only).

iva2k’s picture

I know people don't like to read through the thread (I don't myself).

Here's a condensed list of all changes:
- Track version of Terms and each user's acceptance, force user to accept new version
- Parse more tokens in the checkbox label (@"Node title" in addition to @link)
- CSS injection to limit height of full text of Terms node. Height is configurable
- Add a preview to settings form of how "Tems" fieldset will look on user registration and acceptance forms
- Remove Terms from admin "add user" form. (Untested, but supposedly it will force the user to accept the terms upon first login)
- Checkbox label parser moved to theme_terms_of_use_checkbox_label() function
- Compatible with checkbox_validate, or fixes "required checkbox" bug on its own if checkbox_validate module is not installed
- Add multilingual variables support. Together with multilingual nodes provides full internationalization
- Fixed db_rewrite_sql() failing in multilingual sites (no bug filed yet?)
- Preserve selected node title / node id across AJAX switch between title/id on settings form

- fixed logic that forces new version acceptance form - now it allows nodes linked from Terms node or checkbox token links to be shown (allowed nodes). Without it any links are pointless - logic from versions patch prevents opening any of the links from links patch.
- added hook to recalculate allowed nodes when involved nodes are edited/deleted, or translations added
- added logic to allow terms configuration without a node, can use checkbox link tokens. If neither is present, terms of use are disabled (inactive).
- admin user is not forced to accept new terms
- fieldset name changed to be a title, "use fieldset" checkbox added. Fieldset only applied to user registration form. New version acceptance form uses title, but not the fieldset
- Moved configuration form to .admin.inc file for better performance
- Various wordings improved

- added simplistic views support (expose Terms of Use Version field for user views). No sorting or arguments support yet.
- added couple of special cases - print, printpdf paths (support print.module). Some users may want to print the Terms, or at least open them in a separate window.

I count about 15 individual patches in this list... Wow, if each patch is worked in separately, and it takes 2-3 weeks for a patch to get through (optimistically), it could easily be 45+ weeks.

kars-t’s picture

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

Hi iva2k

thanks for the comprehensive response!

I did look through this patch and I have to say I don't want to use it. You did copy more than 100 lines from the linkchecker module as you stated. So TOU should depend on the linkchecker module but not duplicate it. The patch feels to me like a feature set you need to get your stuff done. And you stated multiple times you don't want to wait.

Sorry but I will turn back to the small patches and fix those issues individually.

Of course I will be happy if you assist me doing that.

iva2k’s picture

"won't fix" - no problem, do as you deem necessary. I rocked the boat enough, so we already getting somewhere :)

I'll probably won't have much time to even try all the little patches one at a time, so I'll be looking from the sidelines how others uncover same problems that I encountered and fixed.

"You did copy more than 100 lines from the linkchecker module as you stated. So TOU should depend on the linkchecker module but not duplicate it."
The problem with making it a dependency is the function I need is configured for the linkchecker use (i.e. variable_get("linkchecker_***") for every option). If you make it a dependency, you'll have to mess up with all these variables, which is more pain than just copy and adopt the code.

"The patch feels to me like a feature set you need to get your stuff done"
Yeah, that's how Drupal works... but
I have a standard set of requirements, like most users would:
1. Versions that force users to re-accept, so I can roll it out on an active website
2. Links in the Terms node
3. Links in the checkbox
4. Multilingual
5. No bugs
Ok, some users don't care for multilingual, but I don't see TOU useful without links AND versions.

"Of course I will be happy if you assist me doing that."
Let me know if you have specific need - I will be glad to help.

lpalgarvio’s picture

subscribing