Yosodog / game

2 stars 1 forks source link

Add/edit/assign/delete roles done #49

Closed LordStrum closed 7 years ago

Yosodog commented 7 years ago
  1. Checkboxes are flippin huge
  2. Panels below and including 'edit role' is not aligned properly. They're too far to the left
  3. In edit role, you have a space in front of the ID in the option's value. Might be leading to some errors
  4. The create role and edit role text input needs to have some placeholder text to say what it's for.
  5. I'd probably move the managing roles into another tab on the edit alliance page. It's starting to get clustered. This can probably be done later, though.
  6. Should probably use gates to check if they are allowed to edit roles. It allows for more reusability.
  7. The create role method really needs to be cleaned up. If the checkbox isn't checked, then it'll be null, right? So try and utilize the null coalescing operator and see what you come up with ;) Hint... something like this:
    $role = Role::create([
     'name' => $this->request->name ?? false,
     // etc...
    ]);

    You can also do the same thing for the edit role method.

Other than that, I'll do some further testing in a day or so.

LordStrum commented 7 years ago

Not going to comment on everything above right now, but attempting to do your fix for create role doesn't work.

$role = Role::create([ 'name' => $this->request->name, 'alliance_id' => $alliance->id, 'canChangeName' => $this->request->nameChange ?? false, 'canRemoveMember' => $this->request->userRemove ?? false, 'canDisbandAlliance' => $this->request->disband ?? false, 'canChangeCosmetics' => $this->request->cosmetics ?? false, 'canCreateRoles' => $this->request->roleCreate ?? false, 'canEditRoles' => $this->request->roleEdit ?? false, 'canRemoveRoles' => $this->request->roleRemove ?? false, 'canReadAnnouncements' => $this->request->announcements ?? false, 'isDefaultRole' => false, 'canAssignRoles' => $this->request->roleAssign ?? false, ]);

just sets everything as false, regardless of the roles being true or not. I'm pretty sure checkboxes return a string, so not sure that'd work. If you can think of a better way to do it, please let me know

Yosodog commented 7 years ago
$role = Role::create([
    'name' => $this->request->name,
    'alliance_id' => $alliance->id,
    'canChangeName' => $this->request->has('nameChange') ?: false, // Works
    'canRemoveMember' => $this->request->userRemove ?? false, // Also works
    // etc
]);

Worked for me :/ If I recall correctly, checkboxes either return a 1 or just literally nothing.

I also noticed that you're not checking server side for the role name to be filled out.

Oh and also if something is just always false, you don't have to have a 'isDefaultRole' => false,, because when you insert it to the database it'll just default to false.

LordStrum commented 7 years ago

the $this->request->has('x'); works, I've added that. Cleaned up the default false stuff as well.

Don't need to check server side because the box is required.

LordStrum commented 7 years ago

Fixed a bunch of stuff with commits, addressed the rest below:

In edit role, you have a space in front of the ID in the option's value. Might be leading to some errors // no idea where this is, point out pls

I'd probably move the managing roles into another tab on the edit alliance page. It's starting to get clustered. This can probably be done later, though. // we can format this shit later

Should probably use gates to check if they are allowed to edit roles. It allows for more reusability. // what's the difference between using a gate and checking if they have that permission like I have been

Yosodog commented 7 years ago

Good job, looks a lot better.

You still need to verify literally everything on the server side because anyone can just inspect element -> remove the 'required' on the input and submit a role without a name. Try it for yourself.

Line 247 on the alliance edit view has the space before the option. I commented on the line for you.

Gates allow us to easily change the verification needed to edit permissions. Lets say shit is changed in the future that changes the way the roles are verified, we just change one method rather than everywhere we need to check if they're allowed to do something. https://laravel.com/docs/5.3/authorization#gates

LordStrum commented 7 years ago

Alright, fixed the ID and the verification.

Yosodog commented 7 years ago

Sounds good to me. Pulling now.