MrJuliuss / syntara

Admin package for Laravel 4.
MIT License
302 stars 77 forks source link

Edit/Show group no longer working #158

Closed securit closed 9 years ago

securit commented 10 years ago

Can no longer click on Show a group as routes like /dashboard/group/7 now come back with a "No data received" dialog. I think something is broken in the controller/view for groups.

MrJuliuss commented 10 years ago

Hello,

It works with my syntara's version. Which version of syntara ? php ? browser ?

securit commented 10 years ago

Ok, so I have tried this with Syntara 1.2.10, php 5.5.16 and 5.5.9-1ubuntu4.4, Chrome Version 37.0.2062.124 & Version 39.0.2169.3 canary (64-bit), Safari Version 7.1 (9537.85.10.17.1). Also when you have a significant number of permissions assigned to a group, the list of permissions may exceed the width of the page, so it can be difficult to get to the "show" button on the right hand side. On Safari, you have to use the cursor to go to the far right. On Chrome, pressing the tab key works once you have worked your way through the other tab stops on the page. It would be nice if the list of permissions wrapped instead.

MrJuliuss commented 10 years ago

Hello, ok i will watch for permissions length. But i did'nt reproduce "No data received" bug =/

MrJuliuss commented 10 years ago

Hi, i haven't any problem with groups permissions with a large screen (tested with Chrome 37 & latest firefox on ubuntu, can you screen me your problem ?). But this list seem to be a problem on an small device. i just fixed it : https://github.com/MrJuliuss/syntara/commit/39efb3cf70d24277f2c8148bd8eb356289f5a59b (it was already applied on users list)

securit commented 10 years ago

Bonjour, I have tracked this down to the call to line 139 of src/controllers/GroupController.php $allUsers = Sentry::getUserProvider()->findAll();

I appear to be running out of memory when retrieving allUsers because my user table is large. I pushed up the php memory and then had maximum execution time errors occurring.

Digging a little deeper on this discovered massive inefficiencies in the userController and the database queries.

All this is because of the way you are creating a list of candidate users and the way Cartalyst interracts with Laravel and the way that Laravel creates its database queries. On line 142 you are calling $user->inGroup for each user. This makes (total number of users * query) calls to the database to see if the user exists in the group. If you have a large number of users like I do, then this is very inefficient and causes the memory and cpu to spike.

So capturing the SQL queries made from Laravel for user ID 14083 I see a query... select groups.*, users_groups.user_id as pivot_user_id, users_groups.group_id as pivot_group_id from groups inner join users_groups on groups.id = users_groups.group_id where users_groups.user_id = 14083 {"bindings":[14083],"time":0.64,"name":"mysql"} []

I think that the list of candidates should probably be paginated too.

MrJuliuss commented 9 years ago

Hi, i see, i will fix it asap, thx ! (sorry for the late)

MrJuliuss commented 9 years ago

Hi, i replace the foreach on all users with only one query :

https://github.com/MrJuliuss/syntara/commit/15078bb728f40d3c920f9157443d7dd3f9ae2ae4

It should fix your problem.

Can you test on dev-master? If it's fixed, i will release asap. Julien