Consultation module review

[Feng-Shui]

Morning all,

I've been asked to do a review of the consultation module, we're interested in using it at some point. My comments are below. I'm happy to open pull requests for any of these.

Thanks for the work on this module!

1: Undefined constant COMMENT_NODE_OPEN

This constant is declared by the comment module and is used when preprocessing consultation nodes. The comment module is not listed as a dependency and so this constant can undefined. It doesn't cause any functional issues, but the code should check if it's defined before comparing against it.

2: Code comments

govcms_consultation_theme_registry_alter() is missing a doc block and a number of the helper functions don't document return types etc. I'm happy to document these.

3: Un-used code

govcms_consultation_form_node_form_alter() has an if condition in it with no code inside, and there are a few un-used variables in govcms_consultation_preprocess_node(): $end_consultation_date and $time.

4: Consultation duration not calculated correctly

_consultation_vars() is calculating the duration of a consultation incorrectly. It’s doing a subtraction between two timestamps which is problematic due to timezones and not being inclusive of the end date:

1451599200 =  1 Jan @ 9:00am
1454220000 = 31 Jan @ 5:00pm

(1454220000 - 1451599200) / 86400 = 30.3 = 30 when rounded

Suggest that this is replace with a date based comparison which ignores time.

5: Namespace for fields and functions

There are a few helper functions which do not have the module's namespace prefix, _consultation_vars(), _consultation_percentage() and _consultation_days_remain(). These should be namespaced inline with the modules project name.

The module also doesn't namespace it's declared fields, and they include some quite generic field names which could foreseeably be used by other modules or site builders: field_icon, field_file_word, field_updates etc.

However, I note that this occurs across a lot of the features, with govcms_events declaring field_cost, field_contact and field_location, govcms_publications declaring field_date etc. Prob not much can be done about this as this stage.

6: Entropy in _govcms_consultation_admin_return_salted_hash()

My crypto foo is totally rubbish, but thought it might be worth mentioning that this function doesn't use a lot of entropy when seeding crc32(). It passes in a hash set at install time drupal_hash_base64(uniqid()) and then the node ID which is available from the source. Two hashes for different nodes might only have a variation in their seed data of 1 character.

No idea if that's an issue, but if so it could be seeded with the modules UUID, or throwing in another random string etc.

7: Spoofing consultation target for submission

It's possible to post submissions against a closed (or any other) consultation by changing the hidden consultation target field. I can see that this has been done because using a token in the webform pulls in the webform's NID not the consultation NID. If the webform was included via code it should be possible to pass the consultation NID through as a parameter and avoid this situation.

Happy to provide a PR on this one, off the top of my head, would look at using drupal_get_form() via a DS field, or possibly using CTools and a NID argument from the path - happy to have a play.

8: Webform accessible on its entity path

The default config has the webform available at it's entity path. The form should probably not be viewed outside of the consultation content as it doesn't make a lot of sense on its own, and, submissions against it wouldn't be tied to any consultation. This could be nailed in hook_init(), return a not found if it's being loaded on it's entity path.

9: Publicly visible submissions nuked by webform clear, email errors?

This still needs some testing this morning, just occurred to me

Regardless of whether the submitter selects to display their submission on the website, Webform Clear deletes submissions immediately. This may be an expectation shortfall if someone is expecting to see their submission once they've made it. In the case that Webform Clear is disabled, Varnish caching might prevent this too, could be something worth putting in the submission confirmation message: "For performance reasons, your submission may take some time to appear".

Might be worth disabling that webform component for the submitter if Webform Clear is enabled?

Re email errors, would be worth testing whether Webform Clear waits to get a response from the webform email action. What happens in the instance where email falls over and the submission is deleted? Prob need some documentation on this one?

[nathan-w]

@Feng-Shui for consultation dates the end time can be very important, depending on the consultation being run

Ideally the content author should be asked to indicate what closing date and time applies, what time zone, and then the system should store the date in UTC. In the example you mention the .3 equates to the 8 hours of 31 Jan the consultation is open and that would expected, and the calculation in my view shouldn't ever be rounded.

Does that change your thoughts? Any other tweaks needed to support this alternative?

Would love to see the PRs for the other things you've raised, they are sound like great enhancements and necessary fixes.

[Feng-Shui]

@nathan-w

Re the consultation end date, I didn't mean to imply that the times should be dropped, because yes, it's really important :)

The code in _consultation_vars() is used in the progress bar to determine how many days remain out of how many in total, so it's effectively time agnostic. My thoughts is that the time is used to figure out the days on which the consultation starts and ends and then those are used for duration - this would eliminate the rounding issue introduced by time.

The dates are stored as timestamps, with no time zone handling enabled, so they're saved using the site's default timezone (from memory). It's been a while since I messed with date time storage so I'd have to double check.

To your point though, the dates and times on the consultation should show timezone (Ends 17:00 AEDST/GMT+10/11 etc), I've sat next to consultation helpdesk staff and that's always a common question. This means it should be selectable on the node forms, or at least a message saying that the selected times are based on the site's currently configured timezone.

I'll dig more into this one today, and see if I can write some test coverage for the date/time stuff.

[nathan-w]

@Feng-Shui Im yet to play with the consultation module IRL - does the progress bar show theres 0 days left when (in this example) its 31 Jan, or does it show there is 1 day left? Doubt Ill get to it today, but Ill have a play with my dev install and get a better understanding of how the progress bar currently works.

An author-selectable time zone with site default fallback would be a good tweak.

[Feng-Shui]

See below for consultation ending today, shows < 1 days remaining. When the consultation is finished, it drops to 0.

[nathan-w]

What if the "Days remaining" became "Hours remaining" during the last day? Message could switch to something like "Closes in 8 hours, 40 minutes"

Understanding of time zones is then irrelevant and people also won't need to interpret what the "<1" actually means.

[Feng-Shui]

I think we'd still need to show the time of day as well. If you say 6 hours and 13 minutes and it's 10:47am, people are going to have to do that maths in their head to figure out when they need to have it done by, and people will get it wrong :)

[nathan-w]

That would still show above the progress bar right?

[Feng-Shui]

Yeah, so I think above the bar stays the same, below the bar would be something like: "Submissions close today: 8 hours 23 minutes remaining"

My comment re having both was that it wouldn't be a replacement for the end date above the bar - sorry that wasn't clear.

[stooit]

Re: timezones, the module doesn't make those assumptions and expects minimal site-building/configuration to suit your requirements. e.g if you want to output the author timezone (or fixed site-wide timezone setting as per The Dept. of Communications and the Arts) you can just configure a date/time format to suit. Using a date format is a better idea, as it will accommodate for DST and change the label (AEST/AEDT) to suit.

The 'Formal submission introduction' field can also be used to provide extra detail per Consultation, possibly a sane place to add information like timezone (especially if the timezones differ per Consultation, or the server timezone doesn't match an organisation location, which are both possibilities).

I like the idea of dropping down to hours in the final day, makes it much clearer as the deadline approaches.

[Feng-Shui]

@srowlands thanks for the input! Was also thinking that a readme would be great for this module. Esp for builders, so you know the webform is a many to one for the consultation nodes, and for config detail like you've mentioned. Do you have anything documented so far?

[stooit]

@Feng-Shui no prob, I was actually fairly time limited in the previous round of effort on this, so didn't get to documentation. I have some local draft content (and thoughts on what would be useful) -- happy to contribute where I can to this.

[stooit]

Re: 9, the module will set the 'Webform submission storage period' to 'Do not delete' for this node. There's not much we can do about email failure unfortunately.

Just submitting also doesn't mean it'll appear immediately, submissions need to be approved for public display before they appear (a private webform field only accessible by a site editor).

Re: 8, yeah that had occurred to me too but not considered a big deal Re: 7, same as above; but would be nice to resolve Re: 6, shouldn't be a big problem. This "lockout" doesn't need to be super secure, it's just to provide a URL for late submissions (and only if that node has the option enabled). It's more important that the crypto checks are performant than super secure here imo Re: 5, this module came from a SaaS govCMS install so it was all built into a theme to begin before it was turned into a module, changing field names would be difficult but no issue in changing the internal function names Re: 4, discussed above Re: 3, sure, minor cleanup exercise Re: 2, sure, minor cleanup exercise Re: 1, ah -- yes. I think the assumption here was that comments were enabled, but checking it's defined may be better than adding the dependency as comments aren't strictly required

[Feng-Shui]

Thanks for the comments. I've got some time over the next few days to take a look at these so I'll get some pull requests done. Also happy to work on a readme if you can sent through any docs/notes that you have.

[stooit]

Sounds good, will do! Thanks @Feng-Shui

[fiasco]

Including the resolution to this issue as the last remaining change for 2.5 which will be the next release to SaaS.

[Feng-Shui]

@srowlands @fiasco Do you want a PR per item above or do you want the whole lot in a single PR? I'm thinking that multiple smaller PRs would be easier to manage, but happy to go either way.

[fiasco]

Agreed, multiple PRs would be preferable as they're easier to accept in on their own merits.

[Feng-Shui]

I'm going to submit one PR that deals with all the mundane stuff, documentation, function namespaces, etc - the stuff that can be reviewed without much effort - otherwise these PRs are going to be a nightmare to merge in terms of conflicts.

4, 7 and 8 are candidates for separate PRs

[Feng-Shui]

https://github.com/govCMS/govCMS/pull/305 is the first PR which is mundane stuff mentioned above. Individual changes are listed against the PR and have been made as individual commits for easier code review.

[aleayr]

Just a note for this thread that this is listed for inclusion on the upcoming 7.x-2.10 release.