refactor server privileges #581

Merged
merged 10 commits into from Aug 11, 2013

Conversation

Projects
None yet
2 participants
Contributor

xmujay commented Aug 9, 2013

No description provided.

Contributor

xmujay commented Aug 9, 2013

Hi Marc.

I have updated the pull request about refactor server privileges.

  1. Move long code to functions in lib file
  2. code style
  3. I have tested in server level and db level
    add user/ delete user/ edit privileges..

lem9 was assigned Aug 9, 2013

Contributor

lem9 commented Aug 9, 2013

Hi Bin,
on my local machine, phpunit has two failures:

1) PMA_ServerUserGroupsTest::testGetHtmlForUserGroupsTableWithNoUserGroups
Failed asserting that '<h2>User groups</h2><fieldset id="fieldset_add_user_group"><a href="server_user_groups.php?addUserGroup=1&amp;token=token"><span class="nowrap"><img src="themes/dot.gif" title="" alt="" class="icon ic_b_usradd" /> </span>Add user group</a></fieldset>' contains "<a href="server_user_groups.php?token=token&addUserGroup=1">".

test/libraries/PMA_server_user_groups_test.php:70

2) PMA_ServerUserGroupsTest::testGetHtmlForUserGroupsTableWithUserGroups
Failed asserting that '<h2>User groups</h2><form name="userGroupsForm" id="userGroupsForm" action="server_privileges.php" method="post"><input type="hidden" name="token" value="token" /><table id="userGroupsTable"><thead><tr><th style="white-space: nowrap">User group</th><th>Server level tabs</th><th>Database level tabs</th><th>Table level tabs</th><th>Action</th></tr></thead><tbody><tr class="odd"><td>usergroup&lt;</td><td>SQL, Status, Users, Export, Import, Settings, Binary log, Replication, Variables, Charsets, Plugins, Engines</td><td>SQL, Search, Query, Export, Import, Operations, Privileges, Routines, Events, Triggers, Tracking, Designer</td><td>Structure, SQL, Search, Insert, Export, Import, Operations, Tracking, Triggers</td><td><a class="" href="server_user_groups.php??viewUsers=1&amp;userGroup=usergroup%3C&amp;token=token"><span class="nowrap"><img src="themes/dot.gif" title="View users" alt="View users" class="icon ic_b_usrlist" /> View users</span></a>&nbsp;&nbsp;<a class="" href="server_user_groups.php??editUserGroup=1&amp;userGroup=usergroup%3C&amp;token=token"><span class="nowrap"><img src="themes/dot.gif" title="Edit" alt="Edit" class="icon ic_b_edit" /> Edit</span></a>&nbsp;&nbsp;<a class="deleteUserGroup ajax" href="server_user_groups.php??deleteUserGroup=1&amp;userGroup=usergroup%3C&amp;token=token"><span class="nowrap"><img src="themes/dot.gif" title="Delete" alt="Delete" class="icon ic_b_drop" /> Delete</span></a></td></tr></tbody></table></form><fieldset id="fieldset_add_user_group"><a href="server_user_groups.php?addUserGroup=1&amp;token=token"><span class="nowrap"><img src="themes/dot.gif" title="" alt="" class="icon ic_b_usradd" /> </span>Add user group</a></fieldset>' contains "<a class="" href="server_user_groups.php?token=token&viewUsers=1&userGroup=usergroup%3C">".

test/libraries/PMA_server_user_groups_test.php:128

Bin,
please explain in which situation these variables can be set.

Please avoid calling functions that change some of their parameters (with the ampersand in the function definition). It's not clear, at the calling point, that such a side effect happens.

Contributor

xmujay commented Aug 9, 2013

Hi Marc,
the UT errors have been fix on my last merged pull request: xmujay/phpmyadmin@d1a70dc#diff-1

Contributor

xmujay commented Aug 10, 2013

Hi Marc,
I have fixed all the issues that you mentioned above. can you help to have one more code review? thanks

Bin,
please explain in which case $Password would be set here.

Owner

xmujay replied Aug 11, 2013

$Password is from https://github.com/phpmyadmin/phpmyadmin/pull/581/files#L1L2976

It is not needed to pass by function.

I have double checked all the functions and remove unneeded parameter and return values.

Bin,
what's the goal of setting $row?

Owner

xmujay replied Aug 11, 2013

$row is just used inner function. I have remove it from return value. thanks

Contributor

xmujay commented Aug 11, 2013

HI Marc,

I have double checked all the functions and remove unneeded parameter and return values.

Tests passed for latest version, thanks

@lem9 lem9 added a commit that referenced this pull request Aug 11, 2013

@lem9 lem9 Merge pull request #581 from xmujay/0809_serverprivileges
refactor server privileges
8e5eb84

@lem9 lem9 merged commit 8e5eb84 into phpmyadmin:master Aug 11, 2013

1 check passed

default The Travis CI build passed
Details
Contributor

lem9 commented Aug 11, 2013

Nice work, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment