Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Surveys for unregistered users #4996

Merged
merged 22 commits into from
Oct 21, 2019
Merged

Surveys for unregistered users #4996

merged 22 commits into from
Oct 21, 2019

Conversation

microstudi
Copy link
Contributor

@microstudi microstudi commented Mar 21, 2019 •
edited
Loading

🎩 What? Why?

In our organization we've come with the necessity to allow everyone to answer certain surveys, whether is registered and logged in the platform or not. We find this specially important to avoid the use of external tools where to place that data (like google forms).

This pull-request implements this feature by adding a new setting option (same as allow_answers) in the survey component admin module (see screenshot). Modules affected ar decidim-forms and decidim-surveys.

In order to make this work I've added a db migration to create a unique token based on the session id of the anonymous user in the decidim_form_answers. This is being used by the data exporter to group all the answers of the user (as the user_id field will be nil in those cases).

[UPDATE]
After discussion, these has been added in order to improve security:

  1. A hashed IP field in the answers, this will allow administrators to detect massive entries without knowing the user IP.
  2. A warning in the admin to prevent admin to use this feature lightly.
  3. Included invisible_captcha
  4. CSV export adds the ip_hash field and the "user status", which tells admins if the answers is coming from a registered or unregistered user.

📌 Related Issues

non related issues

📋 Subtasks

  • Add CHANGELOG entry
  • Add tests

📷 Screenshots (optional)

image

@mrcasals
Copy link
Contributor

@decidim/product what should we do here? I don't know if this has been discussed in MetaDecidim, and I'd like an approval from you before reviewing the code 😄

@microstudi
Copy link
Contributor Author

As I know this may be controversial. I've created in meta-decidim a proposal to discuss the benefits of this feature:

https://meta.decidim.org/processes/roadmap/f/122/proposals/14323

@paarals
Copy link

paarals commented Mar 22, 2019

Hi! I'm with Marc, it's better if we can discuss on meta.decidim. In fact, I think that this depends on #4991? Because before, an external user was not enable to see the survey.

@microstudi
Copy link
Contributor Author

I'd say it's related but not dependant. You can decide to show the survey or to allow answers. The first is useful to let the user know what to expect, and the other to not required them to log in. There's no code coincidences between them.
Yes please, let's discuss on metadecidim.

mrcasals
mrcasals previously requested changes Mar 25, 2019
View reviewed changes
Copy link
Contributor

@mrcasals mrcasals left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm blocking this PR until it is discussed on MetaDecidim. Feel free to discard my review once this is handled! 😄

@microstudi
Copy link
Contributor Author

Hi everyone,
This feature has been approved in metadecidim but they request to add some changes to improve security and avoid spammers.

The changes are:

  1. A clear warning of the risks involved and the obligation to comply with the social contract.
  2. Save the IP address (not whole).
  3. To avoid spam, consider including some invisible captcha.

Now I have some questions about these changes, I would like if any of you could guide me a little for the better way to implement them. My questions are:

  1. I think the easiest way to implement this is by adding a hidden div in the settings option that shows a warning message when the option is activated (will require some javascript). Is there any other example of settings that generate a warning already? if so, then the best would be to replicate it for consistency.
  2. @andreslucena seems to have experience in saving partial IP's in the database. Can you point me where it is done so I can replicate it? thanks a lot.
  3. Is the captcha concept implemented someway in Decidim? Are you using some gem for it? if not, anything you recommend? So far I've found 2 interesting captcha gems in case you can recommend one of them:
    https://github.com/subwindow/negative-captcha
    https://github.com/kiskolabs/humanizer

Thanks a lot in advance!

@andreslucena
Copy link
Member

I think the easiest way to implement this is by adding a hidden div in the settings option that shows a warning message when the option is activated (will require some javascript). Is there any other example of settings that generate a warning already? if so, then the best would be to replicate it for consistency.

We could just add the warning as a message without this logic of hide / show for simplicity.

@andreslucena seems to have experience in saving partial IP's in the database. Can you point me where it is done so I can replicate it? thanks a lot.

As far as I know we don't have this kind of saving partials IP addesses.

Is the captcha concept implemented someway in Decidim? Are you using some gem for it? if not, anything you recommend?

Yes, it's implemented on user registration with https://github.com/markets/invisible_captcha

@microstudi
Copy link
Contributor Author

Thanks @andreslucena for your answers. I'll probably finish this PR during May.

@virgile-dev
Copy link
Contributor

Hey @microstudi
Are you still working on improvements for this feature ?
We have a client that is interested maybe we can help.

@paarals
Copy link

paarals commented Jun 7, 2019 •
edited
Loading

We have a client that is interested maybe we can help.

We too. Our client deserves something like google forms (focusing first on let unregistered participants answer). So, maybe we can built and epic and improve a lot surveys. for example: adding more ways to answer (timetable, etc..).

@microstudi
Copy link
Contributor Author

Yes, still on it!
In fact i've already tested in a production environment, no problems so far. The only thing missing is to implement a couple of security measures (as is been commented).
I expect to complete this PR some day before ending this month.

@stale
Copy link

stale bot commented Aug 7, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. @carolromero & @xabier feel free to chime in.

@stale stale bot added the wontfix label Aug 7, 2019
@stale stale bot closed this Aug 14, 2019
@carolromero
Copy link
Member

I see the bot closed this PR. Are you still working on it @microstudi ?

@carolromero carolromero reopened this Sep 23, 2019
@stale stale bot removed the wontfix label Sep 23, 2019
@microstudi
Copy link
Contributor Author

yes! hopefully soon enough

@carolromero
Copy link
Member

ok! let's keep it open then :)

@microstudi microstudi marked this pull request as ready for review September 30, 2019 14:58
@microstudi microstudi requested a review from a team as a code owner September 30, 2019 14:58
@microstudi
Copy link
Contributor Author

@oriolgual this is ready to review. Thanks

oriolgual
oriolgual previously approved these changes Oct 16, 2019
View reviewed changes
@microstudi microstudi requested a review from mrcasals October 16, 2019 11:20
@mrcasals mrcasals removed their request for review October 17, 2019 11:00
@mrcasals mrcasals dismissed their stale review October 17, 2019 11:01

No longer on the project, I shouldn't be blocking this

@oriolgual oriolgual merged commit 5dffa01 into decidim:master Oct 21, 2019
microstudi added a commit to Platoniq/decidim that referenced this pull request Nov 6, 2019
@microstudi
Surveys for unregistered users (decidim#4996)
e6adced 
* Add allow_unregistered option to surveys component

* locales for allow_unregistered settings action

* relax permissions to allow answers withou users

* Add session token to answer for grouping anonymous answers

* check if an unregistered user has already answered the questionnaire

* add unregistered users test

* rubocop happiness

* change test to match new locale messages

* add changelog entry [ci skip]

* unregisterd users settings warning

* add ip hash to unregistered surveys

* add invisible_captcha to forms

* Add user status to answers export

* remove unused key

* avoid NULL column

* use current instead of now

* refactor questionnaire validation for unregistered users

* simplify questionnaire validation and fix meetings reuse

* fix meetings tests
armandfardeau pushed a commit to OpenSourcePolitics/decidim that referenced this pull request Nov 8, 2019
@microstudi @armandfardeau
Surveys for unregistered users (decidim#4996)
3fd7a27 
* Add allow_unregistered option to surveys component

* locales for allow_unregistered settings action

* relax permissions to allow answers withou users

* Add session token to answer for grouping anonymous answers

* check if an unregistered user has already answered the questionnaire

* add unregistered users test

* rubocop happiness

* change test to match new locale messages

* add changelog entry [ci skip]

* unregisterd users settings warning

* add ip hash to unregistered surveys

* add invisible_captcha to forms

* Add user status to answers export

* remove unused key

* avoid NULL column

* use current instead of now

* refactor questionnaire validation for unregistered users

* simplify questionnaire validation and fix meetings reuse

* fix meetings tests
armandfardeau added a commit to OpenSourcePolitics/decidim that referenced this pull request Nov 14, 2019
@armandfardeau
Surveys for unregistered users (decidim#4996) (#847)
c99eac6 
* Surveys for unregistered users (decidim#4996)

* Add allow_unregistered option to surveys component

* locales for allow_unregistered settings action

* relax permissions to allow answers withou users

* Add session token to answer for grouping anonymous answers

* check if an unregistered user has already answered the questionnaire

* add unregistered users test

* rubocop happiness

* change test to match new locale messages

* add changelog entry [ci skip]

* unregisterd users settings warning

* add ip hash to unregistered surveys

* add invisible_captcha to forms

* Add user status to answers export

* remove unused key

* avoid NULL column

* use current instead of now

* refactor questionnaire validation for unregistered users

* simplify questionnaire validation and fix meetings reuse

* fix meetings tests

* Fix missing translations [ci skip]
@microstudi microstudi mentioned this pull request Jun 3, 2020
Merged
2 tasks
@microstudi microstudi deleted the unregistered-surveys branch July 23, 2020 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrcasals mrcasals mrcasals left review comments

@oriolgual oriolgual oriolgual left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@microstudi @mrcasals @paarals @andreslucena @virgile-dev @carolromero @oriolgual