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.

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

[8381] add option to allow unregistered users to vote in polls #1683

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

goapunk
Copy link
Contributor

@goapunk goapunk commented Oct 29, 2024

Tasks

  • PR name contains story or task reference
  • Documentation (docs and inline)
  • Tests (including n+1 and django_assert_num_queries where applicable)
  • Changelog

@goapunk goapunk changed the title [8381] add option to allow unregistered users to vote in polls WIP: [8381] add option to allow unregistered users to vote in polls Oct 29, 2024
@goapunk
Copy link
Contributor Author

goapunk commented Oct 29, 2024

@m4ra @hom3mad3 here's a version where everything is integrated into the existing poll module for comparison. It's less big of a change than I expected. No changes are necessary on the a+ side so can be tested with main.

@goapunk goapunk force-pushed the jd-2024-10-poll-allow-unregistered branch from e9de720 to 10badee Compare October 29, 2024 10:23
) > 0
return False

def update(self, instance, data):
instance.allow_unregistered_users = data.get("allow_unregistered_users", False)
Copy link
Contributor

Choose a reason for hiding this comment

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

what about the update of the PollViewSet? shouldn't it have a check whether allow_unregistered_users is true? It inherits from the rest framework mixins, so it may need to be overwritten.

Copy link
Contributor Author

@goapunk goapunk Oct 30, 2024

Choose a reason for hiding this comment

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

You mean to check whether a unregistered user trying to vote is allowed to do that ? That's done by the rules/predicates already.

Copy link
Contributor

@m4ra m4ra left a comment

Choose a reason for hiding this comment

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

I left some comments for the django part, but need to do a local test too.
@hom3mad3 can you look more closely to the react part?
BTW can we have the makefile and the github workflows in a separate PR?

@goapunk
Copy link
Contributor Author

goapunk commented Oct 30, 2024

I left some comments for the django part, but need to do a local test too. @hom3mad3 can you look more closely to the react part? BTW can we have the makefile and the github workflows in a separate PR?

I'll make extra PRs but we will have to merge the workflow before this then and rebase it

@m4ra
Copy link
Contributor

m4ra commented Oct 30, 2024

@goapunk tried to test it locally, but we cannot set the allowed_unregistered_users from the poll's dashboard yet. Maybe could be a checkbox in the basic settings? For registered users works as expected.

@goapunk
Copy link
Contributor Author

goapunk commented Oct 30, 2024

@goapunk tried to test it locally, but we cannot set the allowed_unregistered_users from the poll's dashboard yet. Maybe could be a checkbox in the basic settings? For registered users works as expected.

@m4ra it works for me, the checkbox is there (in the Poll setting). Not sure why it's not working for you.

@goapunk goapunk force-pushed the jd-2024-10-poll-allow-unregistered branch 3 times, most recently from 9c27b71 to 818a105 Compare October 30, 2024 13:45
@goapunk goapunk requested a review from m4ra November 4, 2024 08:15
@goapunk goapunk changed the title WIP: [8381] add option to allow unregistered users to vote in polls [8381] add option to allow unregistered users to vote in polls Nov 4, 2024
@goapunk goapunk force-pushed the jd-2024-10-poll-allow-unregistered branch 2 times, most recently from 847551b to 4011c8b Compare November 5, 2024 12:37
Copy link
Contributor

@m4ra m4ra left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@m4ra m4ra merged commit 1796224 into main Nov 5, 2024
3 of 5 checks passed
@m4ra m4ra deleted the jd-2024-10-poll-allow-unregistered branch November 5, 2024 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants