Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
If it's not allowed for everyone to see the results, checkbox "Allow view results", admin can't see the polls results without voting.
We can set "Allow view results" for each poll, that will make results visible without voting, but it will affect all roles (even anonymous if is allowed to see votes).
Proposed resolution
We need to create additional permissions for poll results and separate in per roles. So we can let admin to see the poll results without voting.
Comment | File | Size | Author |
---|---|---|---|
#20 | 2852433-20.patch | 2.52 KB | seanB |
#17 | interdiff_16-17.txt | 1.7 KB | askibinski |
#17 | permission-access-poll-results-2852433-17.patch | 2.3 KB | askibinski |
| |||
#8 | allow_to_see_results_permission-2852433-8.patch | 1.67 KB | Bojan Zivkov |
| |||
#4 | allow_to_see_results_permission-2852433-4.patch | 952 bytes | Bojan Zivkov |
Comments
Comment #2
rteijeiro CreditAttribution: rteijeiro at Tieto commentedHi, any possibility to have this functionality ready?
Is it there anything we can do to speed it up? If someone can provide some suggestions I can allocate resources to work on this.
Comment #3
BerdirA permission to "Always allow to see results" should not be too hard to add, although admin would have that always then. Maybe we could also just assume that users with administer polls are always allowed to see the results.
Most of this logic is in the form class that displays a poll, would likely just need a few additional checks there.
Comment #4
Bojan Zivkov CreditAttribution: Bojan Zivkov at Circle Web Foundry commentedHere is the patch that let admin to see the poll results without voting.
I added new permissions for poll and check for it in PollViewForm class.
Comment #6
Bojan Zivkov CreditAttribution: Bojan Zivkov at Circle Web Foundry commentedSame like https://www.drupal.org/node/2899927 and https://www.drupal.org/node/2884762 ... test keep failing with the same error and i'm unable to debug the test.
Comment #7
BerdirFixed in #2901644: Remeve conflicting AssertContentTrait from PollVoteJavascriptTest.
A test for this would be great. Just extending an existing test, creating a user with that and other necessary permissions and making sure that button shows up should be enough.
Comment #8
Bojan Zivkov CreditAttribution: Bojan Zivkov at Circle Web Foundry commentedHere is the patch for updated existing test with a new permission.
Comment #9
BerdirThat's a start, but now you're only testing that you can grant the permission to a user.
What we should do is only grant the permission the admin user, then you can go to the poll page and assert that the admin can see the View results button.
Comment #10
Bojan Zivkov CreditAttribution: Bojan Zivkov at Circle Web Foundry commentedOk, i see. Here is the updated patch with a new test.
Comment #12
Bojan Zivkov CreditAttribution: Bojan Zivkov at Circle Web Foundry commentedAnd the same patch again without a typo.
Comment #13
BerdirYes, something like that.
Problem is, that is a (slow) javascript test. I think it's better to add this to an existing test method, I'd suggest \Drupal\poll\Tests\PollVoteTest::testPollVote. Give the permission to the admin user *only* in the parent test base class, then first go to the poll with the admin, assert the button exists, then with the normal user and make sure it doesn't
Comment #14
Bojan Zivkov CreditAttribution: Bojan Zivkov at Circle Web Foundry commentedOk, i added permission to admin user (only) in parent class and check for button availability for admin (true) and web user (false). Everything is moved to non-javascript test PollVoteTest::testPollVote.
Comment #16
Bojan Zivkov CreditAttribution: Bojan Zivkov as a volunteer commentedPatch is updated against latest release.
Comment #17
askibinski CreditAttribution: askibinski as a volunteer and at iO commentedPatch didn't apply because version information was comitted in info.yml which is automatically added by the packaging script.
I also made a few improvements to use consistent terminology for the access names and labels.
Comment #18
BrightBoldSomeone who's actually looking at test coverage may want to move the status back, but from an end-user perspective this patch applies successfully and functions as promised.
Comment #19
BerdirSorry for the nitpick but I think we should improve the title here a bit. This permission allows to *always* see the results, I don't think that is clear from the description. People might give that to regular users because they think they can't ever see the results otherwise?
Something like:
title: Always view poll results
descriptions: Can always view poll results without voting, even when that feature is disabled on a poll.
Also, maybe in turn the poll setting description should mention that users with this permission can always view the results (because admins will always have that permission and it might be confusing when testing).
Comment #20
seanBReroll and fixed #19.
Comment #21
LendudeOnly thing I can see here is that it might be nice to align the machinename and the title, but don't care enough to block this, so, looks good
Comment #23
BerdirThanks. Doesn't address the description part that I suggested, but I can live with that. We'll see if someone is confused.
Also, I guess the xpath() could also be nicer now with an elementExists() or so, but it's not as ugly as the escaped HTML check in the other issue, fine :)