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

(dev/core#1104) make admin panels hookable #14734

Merged
merged 1 commit into from Aug 1, 2019

Conversation

@yashodha
Copy link
Member

commented Jul 4, 2019

Overview

Make it easy to show/hide items on administer screen.

Before

not easy to hide elements from admin screen

After

can be done using the hook.

@civibot

This comment has been minimized.

Copy link

commented Jul 4, 2019

(Standard links)

@civibot civibot bot added the master label Jul 4, 2019

@mattwire

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

@yashodha I'm not clear what this allows you to do? Could you provide a screenshot or update description?

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

Documentation issue (& jenkins approval aside) I'm comfortable with adding this hook after seeing the patch - it's not being added in the middle of a complex function but rather at the end as a final change to 'tinker' so it's not locking us into maintaining toxic code around it. It also feels very similar to some or our other patterns around editing available blocks on a page.

So in principle I think this makes sense - once docs & jenkins are addressed

I'm also relaxed about this not having a test as it is very 'close to the surface'

@yashodha

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2019

@mattwire
some admins might not want some roles to see all the overwhelming settings on the admin page. The hook just makes it easy
Before
before

After
after

@yashodha

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2019

test this please

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

@yashodha will you also do a PR to add to the docs repo?

@yashodha

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2019

@eileenmcnaughton sure, I am going to add documentation as well.

@mattwire

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

@yashodha Looking at the code it looks like that admin page is retrieving links from the admin menu but bypassing permission checks? Would it be better to change the way the menu links are retrieved so that only the ones which a role has permission for are displayed?

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

I think this hook is OK- it seems like the sort of place in the code where we have a clear expectation of what the hook will do - ie it will just alter an array & once @yashodha documents it the implications of that array will be clear & documented.

Where I think new hooks are bad is when they are added into toxic code & the hook implementer could tweak any one of a number of things with the expectation the core code won't change in a way that will break their hook.

@yashodha I'm merging this because you said you WOULD document it - I'll leave it to you to make good on that

@eileenmcnaughton eileenmcnaughton merged commit 46d8739 into civicrm:master Aug 1, 2019

1 check passed

default Build finished.
Details
@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

I added this for it civicrm/civicrm-dev-docs#656

@yashodha

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

@eileenmcnaughton Thanks for closing this! Here is the doc link as promised civicrm/civicrm-dev-docs#657

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

thanks @yashodha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.