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

Allow sharees to see results | Implement PERMISSION_RESULTS #1461

Merged
merged 5 commits into from
Jan 31, 2023

Conversation

susnux
Copy link
Collaborator

@susnux susnux commented Jan 22, 2023

Summary

I currently really need the ability to share responses / have multiple users to be able to see the results of a form.
So this implements this feature by adding the available but currently unused permissions to shares and allowing users with permission results to see the submissions.

Screenshot

Not much visually added except for the permission checkbox (later an other checkbox for the edit permission could be added):
share sidebar with permission checkbox per share

Todo

  • Test: ShareApiController
  • Test: ApiController
  • Test: FormsService
  • Integration Tests
  • Frontend

@susnux susnux force-pushed the feat/share-results branch 14 times, most recently from 206085f to 6ad7f27 Compare January 22, 2023 23:44
@susnux susnux changed the title Feat/share results Allow sharing sharees to see results | Implement PERMISSION_RESULTS Jan 22, 2023
@susnux susnux force-pushed the feat/share-results branch 7 times, most recently from fdba674 to 1fcfbee Compare January 23, 2023 02:09
@codecov
Copy link

codecov bot commented Jan 23, 2023

Codecov Report

Merging #1461 (a032c3c) into main (c271b25) will increase coverage by 3.43%.
The diff coverage is 77.31%.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1461      +/-   ##
============================================
+ Coverage     35.47%   38.91%   +3.43%     
- Complexity      532      556      +24     
============================================
  Files            52       53       +1     
  Lines          2224     2295      +71     
============================================
+ Hits            789      893     +104     
+ Misses         1435     1402      -33     

@susnux susnux force-pushed the feat/share-results branch 2 times, most recently from dbf32d6 to e8cad3b Compare January 23, 2023 04:03
@susnux susnux marked this pull request as ready for review January 23, 2023 04:13
@susnux susnux changed the title Allow sharing sharees to see results | Implement PERMISSION_RESULTS Allow sharees to see results | Implement PERMISSION_RESULTS Jan 23, 2023
@Chartman123
Copy link
Collaborator

On a shared form with the results permission the TopBar shows the "Share" button on first load. Clicking it does nothing and the button disappears:
grafik

@susnux susnux force-pushed the feat/share-results branch 2 times, most recently from 883b241 to 703f2a7 Compare January 25, 2023 13:57
@susnux
Copy link
Collaborator Author

susnux commented Jan 25, 2023

On a shared form with the results permission the TopBar shows the "Share" button on first load. Clicking it does nothing and the button disappears: 🖼️

This was due to wrong "share permissions" on the frontend, fixed that

appinfo/routes.php Outdated Show resolved Hide resolved
docs/API.md Outdated Show resolved Hide resolved
@susnux susnux force-pushed the feat/share-results branch 5 times, most recently from 872d099 to 7cf945c Compare January 28, 2023 21:47
@susnux
Copy link
Collaborator Author

susnux commented Jan 28, 2023

I resolved the comments and added an additional integration test for updating share permissions.

Copy link
Collaborator

@Chartman123 Chartman123 left a comment

Choose a reason for hiding this comment

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

@susnux From my point of view this is ready. Just try to cleanup your commits by squashing the fix commits into the original commits. 🙂

Then let's wait for @jotoeri to approve this and get this merged afterwards 👍🏻

Copy link
Member

@jotoeri jotoeri left a comment

Choose a reason for hiding this comment

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

Just one small doc remaining, code should be ok.

  • But let's first get Fix insert #1465 merged and released. Then we can adapt the API calmly afterwards.

docs/API.md Outdated Show resolved Hide resolved
@susnux susnux force-pushed the feat/share-results branch 2 times, most recently from f6a347f to ae2d903 Compare January 29, 2023 12:24
@susnux
Copy link
Collaborator Author

susnux commented Jan 29, 2023

Just one small doc remaining, code should be ok.
But let's first get #1465 merged and released. Then we can adapt the API calmly afterwards.

Fixed that :)
I would have waited for that PR to get merged before anyways, so we could release a fix release quickly before implementing new features :)

@susnux susnux requested a review from jotoeri January 29, 2023 12:27
@susnux susnux added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jan 29, 2023
Allow users with `results` permission to query and export results

Signed-off-by: Ferdinand Thiessen <[email protected]>
* Add unit tests in `FormsService` and `ShareApiControllerTest`
* Add permissions to share API documentation

Signed-off-by: Ferdinand Thiessen <[email protected]>
Signed-off-by: Ferdinand Thiessen <[email protected]>
Check permission on frontend to show delete buttons

Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux susnux merged commit 842365e into main Jan 31, 2023
@susnux susnux deleted the feat/share-results branch January 31, 2023 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement New feature or request feature: 📊 responses & statistics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to share responses / results
4 participants