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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Bojan Živkov created an issue. See original summary.

rteijeiro’s picture

Hi, 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.

Berdir’s picture

A 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.

Bojan Zivkov’s picture

Here 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.

Status: Needs review » Needs work

The last submitted patch, 4: allow_to_see_results_permission-2852433-4.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Bojan Zivkov’s picture

Status: Needs work » Needs review

Same 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.

Berdir’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Fixed 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.

Bojan Zivkov’s picture

Status: Needs work » Needs review
FileSize
1.67 KB

Here is the patch for updated existing test with a new permission.

Berdir’s picture

Status: Needs review » Needs work

That'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.

Bojan Zivkov’s picture

Status: Needs work » Needs review
FileSize
2.13 KB

Ok, i see. Here is the updated patch with a new test.

Status: Needs review » Needs work

The last submitted patch, 10: allow_to_see_results_permission-2852433-10.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Bojan Zivkov’s picture

Status: Needs work » Needs review
FileSize
2.26 KB

And the same patch again without a typo.

Berdir’s picture

Status: Needs review » Needs work

Yes, 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

Bojan Zivkov’s picture

Status: Needs work » Needs review
FileSize
2.32 KB

Ok, 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.

Status: Needs review » Needs work

The last submitted patch, 14: allow_to_see_results_permission-2852433-14.patch, failed testing. View results

Bojan Zivkov’s picture

Version: 8.x-1.1 » 8.x-1.2
Status: Needs work » Needs review
FileSize
2.89 KB

Patch is updated against latest release.

askibinski’s picture

Patch 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.

BrightBold’s picture

Status: Needs review » Reviewed & tested by the community

Someone 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.

Berdir’s picture

Version: 8.x-1.2 » 8.x-1.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs tests
+++ b/poll.permissions.yml
@@ -11,3 +11,5 @@ access poll overview:
   title: 'Cancel own vote'
+view poll results:
+  title: 'View poll results'

Sorry 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).

seanB’s picture

Status: Needs work » Needs review
FileSize
2.52 KB

Reroll and fixed #19.

Lendude’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/poll.permissions.yml
@@ -11,3 +11,6 @@ access poll overview:
+view poll results:
+  title: 'Always view poll results'

Only 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

  • Berdir committed c4f7358 on 8.x-1.x authored by seanB
    Issue #2852433 by Bojan Živkov, askibinski, seanB: Missing permission...
Berdir’s picture

Status: Reviewed & tested by the community » Fixed

Thanks. 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 :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.