Module description
Views Tag Access provides granular permission control for the administration of views based on view tags.
Site administrators can configure a list of tags which the module will create permissions for the main operations (update/enable/disable/duplicate/delete) as well as all operations and an administer all views
permission.
We then use hook_entity_access()
to deny access to administrative operation for any view which the user doesn't have permission to and we override \Drupal\views_ui\ViewListBuilder
to only display the views we have access to.
Git clone command
git clone --branch 8.x-1.x https://git.drupal.org/sandbox/andrewbelcher/2650960.git views_tag_access
Project application reviews
https://www.drupal.org/node/2636002#comment-10766854
https://www.drupal.org/node/2573077#comment-10767562
https://www.drupal.org/node/2462853#comment-10768192
Comments
Comment #2
PA robot CreditAttribution: PA robot commentedThere are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxandrewbelcher2650960git
We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)
Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).
I'm a robot and this is an automated message from Project Applications Scraper.
Comment #3
andrewbelcher CreditAttribution: andrewbelcher at FreelyGive commentedFixed bits flagged up by the PA Review and made the permissions work a bit better.
Comment #4
andrewbelcher CreditAttribution: andrewbelcher at FreelyGive commentedAdded an application review link.
Comment #5
andrewbelcher CreditAttribution: andrewbelcher at FreelyGive commentedAdded another review link.
Comment #6
andrewbelcher CreditAttribution: andrewbelcher at FreelyGive commentedAdded another review and tagged for the review bonus.
Comment #7
yanniboi CreditAttribution: yanniboi at FreelyGive commentedThis looks pretty cool!
I haven't done a full review, but I have looked through the code and installed it and looked through the UI.
It works, is intuitive and will probably be quite useful for stopping users without views training accidentally breaking the site :)
Comment #8
almaudoh CreditAttribution: almaudoh commented@andrewbelcher, I have checked through your code, it's a great module with no real code issues. Just some small nitpicks here:
This function does some kind of magic, a comment here would be useful (in case someone else in future is maintaining the code :))
The class doc doesn't seem to match the class functionality.
Edit: also in views_tag_access.module, for easier readability, you don't need to use fully namespaced class names (e.g. \Drupal\Core\Access\AccessResult::neutral()) in code, you can just place a
use \Drupal\Core\Access\AccessResult;
statement at the top of the file after the@file
docblock and then use the class name only.Comment #9
andrewbelcher CreditAttribution: andrewbelcher at FreelyGive commented@almaudoh thanks for the review.
I've updated the documentation for
views_tag_access_module_implements_alter()
, fixed a few copy/paste documentation issues (such as the ones you spotted) and also switch to ause
statement forAccessResult
(I don't think I was expecting it to be used so often inviews_tag_access.module
when I decided not to use ause
).Would you be up for doing a 'full' review so this can be marked as RTBC?
Comment #10
rlhawkThis module is very useful and worked as advertised in my manual tests. A permission-based module such as this would be well-suited to automated tests though, so give some consideration to adding simple tests to make sure permissions settings are being respected.
You should probably mention in the documentation that in order for the permissions to have an affect, the "Administer views" permission needs to be disabled for the role that should be restricted.
A link to the configuration page from the modules page would be helpful. Add
configure: views_tag_access.settings
to views_tag_access.info.ymlAutomated Review
There are a handful of (generally minor) coding standards issues. Evidently, "Namespaced classes, interfaces and traits should not begin with a file doc comment" is a fairly new change to the standards (see #2304909: Relax requirement for @file when using OO Class or Interface per file).
Note that perfect adherence to Drupal Coding Standard is NOT a reason to block an application, except for total disregard of them. However, modules should follow them as closely as possible.
Manual Review
The text in the README documentation for the project is thorough, except for the item about the "Administer views" permission mentioned above. It would be helpful if some, or all, of the information from the readme file were included on the project description page also.
If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.
This review uses the Project Application Review Template.
Comment #11
klausi@rlhawk: I think you forgot to change the status. Are there any blockers left or is this now RTBC?
Comment #12
rlhawkYes, RTBC, though I hope my suggestions about tests, documentation, and usability improvements will be considered.
Comment #13
mpdonadioLooking at this now.Will look at this as soon as packagist is back up.Comment #14
andrewbelcher CreditAttribution: andrewbelcher at FreelyGive commented@rlhawk I think automated test would be great and I think your documentation changes would be helpful. I'll try and get those in this week.
@mpdonadio thank you!
Comment #15
mpdonadioAutomated Review
Review of the 8.x-1.x branch (commit 2904809):
This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.
Manual Review
Don't worry about wrappng code to 80 cols, just comments.
You have a buch of fully qualified classes; use statements are more standard.
You have some places in OO code where you access \Drupal. Use DI instead.
Nifty module. Assigning to @klausi for a second look from a security perspective, but this looks good to me. If he doesn't have time w/in 7 days, I'll do the vetting.
If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.
This review uses the Project Application Review Template.
Comment #16
andrewbelcher CreditAttribution: andrewbelcher at FreelyGive commentedI've updated the documentation and code style bits - see new PA review.
Comment #17
andrewbelcher CreditAttribution: andrewbelcher at FreelyGive commentedI've added tests for the access alterations and created some issues for other test coverage and removing the use of
\Drupal
in OO classes.Comment #18
andrewbelcher CreditAttribution: andrewbelcher at FreelyGive commentedI've also fixed the DI issue!
Comment #19
klausimanual review:
Otherwise looks good to me.
Thanks for your contribution, Andrew!
I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.
Here are some recommended readings to help with excellent maintainership:
You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!
Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.
Thanks to the dedicated reviewer(s) as well.
Comment #20
andrewbelcher CreditAttribution: andrewbelcher at FreelyGive commented@klausi thanks! I've added the additional documentation.