Relates to : T1407
Details
Details
- Reviewers
epriestley - Maniphest Tasks
- T1407: Feature Suggestion : Admin LDAP import on people page
(With ldap auth available)
Log in as admin
Go to people page
Click import from ldap
Enter an ldap username and password as well as a query string.
Select users to be imported
Click import.
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
Sorry for the delay on this. Looks generally good, mostly just a lot of style nitpicks.
Lint raises these errors, can you fix them? If you run "arc lint" it should be able to fix many of them automatically, although you'll need to build xhpast for it to work and that might be difficult. Most of this is just trailing whitespace.
>>> Lint for src/applications/auth/ldap/PhabricatorLDAPProvider.php:
Auto-Fix (TXT6) Trailing Whitespace
This line contains trailing whitespace.
53 public function retrieveUserEmail() {
54 return $this->userData['mail'][0];
55 }
>>> - 56
+
57 public function retrieveUserRealName() {
58 return $this->retrieveUserRealNameFromData($this->userData);
59 }
Warning (XHP26) Space After Control Statement
Convention: put a space after control statements.
173
174 $rows = array();
175
+
176 for($i = 0; $i < $entries['count']; $i++) {
177 $row = array();
178 $entry = $entries[$i];
Auto-Fix (TXT6) Trailing Whitespace
This line contains trailing whitespace.
176 for($i = 0; $i < $entries['count']; $i++) {
177 $row = array();
178 $entry = $entries[$i];
>>> - 179 // Get username, email and realname
+ // Get username, email and realname
180 $username = $entry[$this->getSearchAttribute()][0];
181 if(empty($username)) {
182 continue;
Warning (XHP26) Space After Control Statement
Convention: put a space after control statements.
178 $entry = $entries[$i];
179 // Get username, email and realname
180 $username = $entry[$this->getSearchAttribute()][0];
+
181 if(empty($username)) {
182 continue;
183 }
Auto-Fix (TXT6) Trailing Whitespace
This line contains trailing whitespace.
183 }
184 $row[] = $username;
185 $row[] = $entry['mail'][0];
>>> - 186 $row[] = $this->retrieveUserRealNameFromData($entry);
+ $row[] = $this->retrieveUserRealNameFromData($entry);
187
188
189 $rows[] = $row;
>>> Lint for src/applications/people/controller/PhabricatorPeopleLdapController.php:
Auto-Fix (TXT6) Trailing Whitespace
This line contains trailing whitespace.
74 ->appendChild(
75 id(new AphrontFormPasswordControl())
76 ->setLabel('Password')
>>> - 77 ->setName('password'))
+ ->setName('password'))
78 ->appendChild(
79 id(new AphrontFormTextControl())
80 ->setLabel('LDAP query')
Warning (TXT3) Line Too Long
This line is 92 characters long, but the convention is 80 characters.
79 id(new AphrontFormTextControl())
80 ->setLabel('LDAP query')
81 ->setName('query'))
>>> 82 ->setAction($request->getRequestURI()->alter('search', 'true')->alter('import', null))
83 ->appendChild(
84 id(new AphrontFormSubmitControl())
85 ->setValue('Search'));
Auto-Fix (TXT6) Trailing Whitespace
This line contains trailing whitespace.
88 $panel->setHeader('Import Ldap Users');
89 $panel->appendChild($form);
90
>>> - 91
+
92 if($request->getStr('import')) {
93 $panels[] = $this->processImportRequest($request);
94 }
Warning (XHP26) Space After Control Statement
Convention: put a space after control statements.
89 $panel->appendChild($form);
90
91
+
92 if($request->getStr('import')) {
93 $panels[] = $this->processImportRequest($request);
94 }
Auto-Fix (TXT6) Trailing Whitespace
This line contains trailing whitespace.
92 if($request->getStr('import')) {
93 $panels[] = $this->processImportRequest($request);
94 }
>>> - 95
+
96 $panels[] = $panel;
97
98 if($request->getStr('search')) {
Warning (XHP26) Space After Control Statement
Convention: put a space after control statements.
95
96 $panels[] = $panel;
97
+
98 if($request->getStr('search')) {
99 $panels[] = $this->processSearchRequest($request);
100 }
Auto-Fix (TXT6) Trailing Whitespace
This line contains trailing whitespace.
105
106 private function processImportRequest($request) {
107 $admin = $request->getUser();
>>> - 108 $usernames = $request->getArr('usernames');
+ $usernames = $request->getArr('usernames');
109 $emails = $request->getArr('email');
110 $names = $request->getArr('name');
111
Auto-Fix (TXT6) Trailing Whitespace
This line contains trailing whitespace.
106 private function processImportRequest($request) {
107 $admin = $request->getUser();
108 $usernames = $request->getArr('usernames');
>>> - 109 $emails = $request->getArr('email');
+ $emails = $request->getArr('email');
110 $names = $request->getArr('name');
111
112 $panel = new AphrontErrorView();
Auto-Fix (TXT6) Trailing Whitespace
This line contains trailing whitespace.
107 $admin = $request->getUser();
108 $usernames = $request->getArr('usernames');
109 $emails = $request->getArr('email');
>>> - 110 $names = $request->getArr('name');
+ $names = $request->getArr('name');
111
112 $panel = new AphrontErrorView();
113 $panel->setSeverity(AphrontErrorView::SEVERITY_NOTICE);
Auto-Fix (TXT6) Trailing Whitespace
This line contains trailing whitespace.
108 $usernames = $request->getArr('usernames');
109 $emails = $request->getArr('email');
110 $names = $request->getArr('name');
>>> - 111
+
112 $panel = new AphrontErrorView();
113 $panel->setSeverity(AphrontErrorView::SEVERITY_NOTICE);
114 $panel->setTitle("Import Successful");
Auto-Fix (TXT6) Trailing Whitespace
This line contains trailing whitespace.
112 $panel = new AphrontErrorView();
113 $panel->setSeverity(AphrontErrorView::SEVERITY_NOTICE);
114 $panel->setTitle("Import Successful");
>>> - 115 $errors = array("Successfully imported users from ldap");
+ $errors = array("Successfully imported users from ldap");
116
117
118 foreach($usernames as $username) {
Warning (XHP26) Space After Control Statement
Convention: put a space after control statements.
115 $errors = array("Successfully imported users from ldap");
116
117
+
118 foreach($usernames as $username) {
119 $user = new PhabricatorUser();
120 $user->setUsername($username);
Auto-Fix (TXT6) Trailing Whitespace
This line contains trailing whitespace.
119 $user = new PhabricatorUser();
120 $user->setUsername($username);
121 $user->setRealname($names[$username]);
>>> - 122
+
123 $email_obj = id(new PhabricatorUserEmail())
124 ->setAddress($emails[$username])
125 ->setIsVerified(1);
Error (TXT2) Tab Literal
Configure your editor to use spaces for indentation.
127 id(new PhabricatorUserEditor())
128 ->setActor($admin)
129 ->createNewUser($user, $email_obj);
>>> 130
131 $ldap_info = new PhabricatorUserLDAPInfo();
132 $ldap_info->setLDAPUsername($username);
133 $ldap_info->setUserID($user->getID());
Warning (SPELL0) Possible spelling mistake
Possible spelling error. You wrote 'succesfully', but did you mean
'successfully'
132 $ldap_info->setLDAPUsername($username);
133 $ldap_info->setUserID($user->getID());
134 $ldap_info->save();
>>> 135 $errors[] = 'Succesfully added ' . $username;
^
136 } catch (Exception $ex) {
137 $errors[] = 'Failed to add ' . $username . ' ' . $ex->getMessage();
Auto-Fix (TXT6) Trailing Whitespace
This line contains trailing whitespace.
136 } catch (Exception $ex) {
137 $errors[] = 'Failed to add ' . $username . ' ' . $ex->getMessage();
138 }
>>> - 139 }
+ }
140
141 $panel->setErrors($errors);
142 return $panel;
Auto-Fix (TXT6) Trailing Whitespace
This line contains trailing whitespace.
139 }
140
141 $panel->setErrors($errors);
>>> - 142 return $panel;
+ return $panel;
143
144 }
145
Warning (XHP9) Naming Conventions
Follow naming conventions: variables should be named using
lowercase_with_underscores.
153 $search = $request->getStr('query');
154
155 try {
>>> 156 $ldapProvider = new PhabricatorLDAPProvider();
157 $ldapProvider->auth($username, $password);
158 $results = $ldapProvider->search($search);
159 foreach($results as $key => $result) {
Warning (XHP9) Naming Conventions
Follow naming conventions: variables should be named using
lowercase_with_underscores.
154
155 try {
156 $ldapProvider = new PhabricatorLDAPProvider();
>>> 157 $ldapProvider->auth($username, $password);
158 $results = $ldapProvider->search($search);
159 foreach($results as $key => $result) {
160 $results[$key][] = $this->renderUserInputs($result);
Warning (XHP9) Naming Conventions
Follow naming conventions: variables should be named using
lowercase_with_underscores.
155 try {
156 $ldapProvider = new PhabricatorLDAPProvider();
157 $ldapProvider->auth($username, $password);
>>> 158 $results = $ldapProvider->search($search);
159 foreach($results as $key => $result) {
160 $results[$key][] = $this->renderUserInputs($result);
161 }
Warning (XHP26) Space After Control Statement
Convention: put a space after control statements.
156 $ldapProvider = new PhabricatorLDAPProvider();
157 $ldapProvider->auth($username, $password);
158 $results = $ldapProvider->search($search);
+
159 foreach($results as $key => $result) {
160 $results[$key][] = $this->renderUserInputs($result);
161 }
Warning (TXT3) Line Too Long
This line is 97 characters long, but the convention is 80 characters.
172 '',
173 ));
174 $form->appendChild($table);
>>> 175 $form->setAction($request->getRequestURI()->alter('import', 'true')->alter('search', null))
176 ->appendChild(
177 id(new AphrontFormSubmitControl())
178 ->setValue('Import'));
Auto-Fix (TXT6) Trailing Whitespace
This line contains trailing whitespace.
188 return $panel;
189
190 }
>>> - 191
+
192 private function renderUserInputs($user) {
193 $username = $user[0];
194 $inputs = phutil_render_tag(
Auto-Fix (TXT6) Trailing Whitespace
This line contains trailing whitespace.
196 array(
197 'type' => 'checkbox',
198 'name' => 'usernames[]',
>>> - 199 'value' =>$username,
+ 'value' =>$username,
200 ),
201 '');
202
Auto-Fix (TXT6) Trailing Whitespace
This line contains trailing whitespace.
205 array(
206 'type' => 'hidden',
207 'name' => "email[$username]",
>>> - 208 'value' =>$user[1],
+ 'value' =>$user[1],
209 ),
210 '');
211
Auto-Fix (TXT6) Trailing Whitespace
This line contains trailing whitespace.
208 'value' =>$user[1],
209 ),
210 '');
>>> - 211
+
212 $inputs .= phutil_render_tag(
213 'input',
214 array(
Auto-Fix (TXT6) Trailing Whitespace
This line contains trailing whitespace.
214 array(
215 'type' => 'hidden',
216 'name' => "name[$username]",
>>> - 217 'value' =>$user[2],
+ 'value' =>$user[2],
218 ),
219 '');
220| src/applications/people/controller/PhabricatorPeopleLdapController.php | ||
|---|---|---|
| 26–30 | Unused? | |
| 34–40 | All unused? | |
| 42–48 | Consider moving the body of processBasicRequest() here since there are no other request types for this controller. | |
| 80 | Although I'm not familiar with LDAP, I have no clue what I should put in this field. Maybe add an example with setCaption()? | |
| 82 | Move this up top rather than in the middle of the fields. | |
| 88 | We should be consistent about "Ldap" or "LDAP" -- maybe "LDAP"? | |
| src/applications/people/controller/PhabricatorPeopleListController.php | ||
| 138 | For consistency, prefer trailing slash (/people/ldap/). The routing code will add one if you omit it, but it costs a redirect. | |
Comment Actions
Thanks for that Evan,
Fixed the style issues and refactored away the superfluous method.