|
Notes |
|
|
(0003914)
|
|
Dmitry
|
|
2011-09-26 08:24
|
|
|
We have it in Resumark for sure! |
|
|
|
(0004007)
|
|
Dmitry
|
|
2011-10-04 20:18
|
|
Erik, please review and take from Pasttimes project.
Make sure to add new HIDE new button in Simple System preset |
|
|
|
(0004032)
|
|
erik
|
|
2011-10-06 10:05
|
|
|
patch attached - needs testing |
|
|
|
(0004035)
|
|
alex
|
2011-10-06 11:18
(edited on: 2011-10-06 14:18) |
|
1. There is no need for getFirstSelected JavaScript method, since Grid class already has a method to get all selected IDs in grid. Grid class object can be accesed via Grids['prefix.special'] construct. I've created a "code review" (in the past) for you about another project with exactly same problem.
2. There is no need to add additional parameters to m_Link tag result, when they can be incorporated within it. Please move user_event into m_Link tag parameters. Also there is no need to use "events[u]" form for event setting (please use u_event standard way).
3. Not all phrases used in this patch have been added into language pack (english.lang file).
4. All phrases in language pack should be alphabetically sorted. I've reported this problem in another 2 tasks already.
5. Please use method of verifying admin session, used for "Browse Mode" processing (the one in kApplication::Run method for detecting if admin is logged-in or not) and not create new method, like you did with _hasAdminSession.
6. Using loginUserById method alone to login user won't properly set it's groups to session. Please look into ActivateUser tag to find a way how it should be properly done.
|
|
|
|
(0004039)
|
|
erik
|
|
2011-10-11 09:31
|
|
|
login_as_user_1105.2.patch attached - needs testing |
|
|
|
(0004040)
|
|
alex
|
|
2011-10-11 10:00
|
|
1. Please don't try to create non-obvious code, where it can be obvious.
For example code
if (Grids['u.regular'].GetSelected()[1]) {
should be literally checking if there was more then 1 user selected, then why checking for 2nd array element presence, when direct check can be made (which looks more obvious to the one who don't know how it should work):
if ( Grids['u.regular'].GetSelected().length > 1 ) {
2. Don't just copy/paste code, when there could a way to do it more effieciently, e.g. by using already existing method to do the job. For example code:
$user = (int)$admin_session->RecallVar('user_id');
return ($user != 0);
could be replaced with:
return $admin_session->LoggedIn();
3. Don't create intermidiate variables, when you plan to use then only once, e.g. "$user" variable from example above.
4. If variable contains some ID in it, then it should indicate this in it's name, e.g. "$user_id" instead of "$user". You can see other parts of In-Portal code to get the idea.
5. Tabulation in OnLoginAs method is completely wrong (see attached patch to get the idea if PHP Editor you're using donesn't provide a quick way to see what's wrong).
6. Some of the phrases in the language pack doesn't belong to "Core" module. Please make sure to export only "Core" module phrases of "Admin" & "Both" types on language pack export dialog. This way you'll notice that phrase isn't exported soon enough. |
|
|
|
(0004041)
|
|
erik
|
|
2011-10-11 12:31
|
|
|
login_as_user_1105.3.patch attached - needs testing |
|
|
|
(0004045)
|
|
alex
|
|
2011-10-12 05:57
|
|
|
Patch "login_as_user_v4.patch" also fixes issue with all home page links to result in 404 page in mod-rewrite when "env" variable is present. |
|
|
|
(0004046)
|
|
alex
|
|
2011-10-12 05:57
|
|
Fix committed to 5.2.x branch. Commit Message:
Fixes 0001105: Add "Login as User" button to user list in Admin
Commit on behalf of Erik |
|
|
|
(0005113)
|
|
alex
|
|
2012-07-25 05:33
|
|
|
Since 5.2.0 version was released. |
|
login_as_user_1105.patch (6,842) 2011-10-06 10:05