-
Notifications
You must be signed in to change notification settings - Fork 2
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
Modify backpack to allow league admin event control #405
Modify backpack to allow league admin event control #405
Conversation
Code Climate has analyzed commit 4490ea9 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 96.4% (90% is the threshold). This pull request will bring the total coverage in the repository to 92.2% (0.5% change). View more on Code Climate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple things i noticed
…lete to ensure they are allowed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After looking at this more closely, i realized theres a whole settings / access API in the crud panel that we're not leveraging which would make this much simpler.
The way we'd use it is pretty straight forward, in the accessible trait, you'd change the method implementation to look like this:
if (!Auth::user()->isSiteAdmin()) {
$this->crud->denyAccess(['create', 'show', 'list', 'reorder', 'delete']);
}
Then for the two crud panels that users have access too you just need to filter the lists and deny access on the show / list, much like you're doing now. you can keep as a abort(403), but its probably better to call $this->crud->denyAction('show')
/ $this->crud->denyAction('list')
, etc.. If you dont follow, we can schedule a call to talk about it
app/Models/User.php
Outdated
public function isLeagueAdminRole(): bool | ||
{ | ||
return $this->hasRole(Role::LEAGUE_ADMIN); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be preferable to have modified the function below this one to allow no $leagueId
to be passed and just check if they are a league admin at all
resources/views/vendor/backpack/base/inc/sidebar_content.blade.php
Outdated
Show resolved
Hide resolved
…rud controller
… crud controller
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow! very good work.
I'm not sure if this is by deisgn or if im missing something here.
I did the following steps to get to the screenshot:
- registered a new account (so i can log in / out)
- logged in as site admin and attached league admin role to user
- logged out and then back in as my test acc
- verified that I can see the admin panel (and only saw the event and sessions sidebar)
- next i tried creating an event, this is where im at in the screenshot
As you can see I have no leagues available to me in the drop down. This is because league id is a pivot value and rn theres no way to modify that through BP i think. (thats fine)
The problem is that despite not technically being an admin of any league, and just having a blank league admin role, I can select sessions that already exist elsewhere (that i did not create) to be part of my event.
I was not able to submit the request as the league field wasnt possible to fill and so the form validation didnt pass. I suspect tho that even if I had some league admin role with a league id pivot value, id still be able to select other leagues' sessions for my event. I also suspect that I'd then be able to submit that request and have those sessions be tied to my event.
Im not quite sure how to fix this or what changes to be made, but I think it is a problem that should be fixed before this is merged.
(attaching the league id pivot on user league attachment is possibly out of scope of this task, not sure though)
…to access backpack
…tionality to similar function
…its functionality to similar function" This reverts commit 2c22f10.
…as a league id
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright i think we've plugged pretty much any holes in this, seems solid now :) good work!
Description
Closes #374 #421