Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ushashree_m created an issue. See original summary.

ushashree_m’s picture

This patch will add the user search.

Albert Volkman’s picture

  1. +++ b/userpoints.admin.inc
    @@ -372,6 +372,13 @@ function userpoints_admin_txn_submit($form, &$form_state) {
    +function userpoints_admin_points_callback() {
    

    Missing docblock.

  2. +++ b/userpoints.admin.inc
    @@ -372,6 +372,13 @@ function userpoints_admin_txn_submit($form, &$form_state) {
    +  $output = array();
    +  $output['search'] = drupal_get_form('userpoints_search_user_form');
    

    This could more simply be written as-

    return array(
      'search' => drupal_get_form('userpoints_search_user_form'),
      'form' => drupal_get_form('userpoints_admin_points'),
    );
    
  3. +++ b/userpoints.admin.inc
    @@ -395,6 +402,9 @@ function userpoints_admin_points($form, &$form_state) {
    +    $query->condition('u.name', '%' . db_like($_GET['name']) . '%', 'LIKE');
    

    Is the addcslashes() added by db_like() sufficient in escaping user input?

  4. +++ b/userpoints.admin.inc
    @@ -809,3 +819,55 @@ function userpoints_admin_settings($form, &$form_state) {
    +  $form['user_name'] = array(
    ...
    +  $form['submit'] = array(
    

    I'd recommend wrapping these in a fieldset, as it'll be more visually appealing.

  5. +++ b/userpoints.admin.inc
    @@ -809,3 +819,55 @@ function userpoints_admin_settings($form, &$form_state) {
    + * Implements hook_form_submit.
    

    Refer to https://www.drupal.org/coding-standards/docs#forms for documenting forms and their validation/submission handlers.

  6. +++ b/userpoints.admin.inc
    @@ -809,3 +819,55 @@ function userpoints_admin_settings($form, &$form_state) {
    +  if(isset($form_state['values']['user_name']) && !empty($form_state['values']['user_name']))
    

    "if" should be followed by a space. Also, always use curly braces for conditionals- https://www.drupal.org/coding-standards#controlstruct

  7. +++ b/userpoints.admin.inc
    @@ -809,3 +819,55 @@ function userpoints_admin_settings($form, &$form_state) {
    +  drupal_goto("admin/config/people/userpoints", $queryargs);
    

    Using a drupal_goto() feels incorrect here... Would $form_state['redirect'] work?

  8. +++ b/userpoints.admin.inc
    @@ -809,3 +819,55 @@ function userpoints_admin_settings($form, &$form_state) {
    + * autocomplete helper
    + * $string = string for search
    

    This docblock needs to be cleaned up.

  9. +++ b/userpoints.admin.inc
    @@ -809,3 +819,55 @@ function userpoints_admin_settings($form, &$form_state) {
    +  $query = db_select('userpoints', 'p')
    

    I'd prefer to see userpoints_admin_points() refactored to abstract the userpoints model as opposed to creating a new query here.

  10. +++ b/userpoints.admin.inc
    @@ -809,3 +819,55 @@ function userpoints_admin_settings($form, &$form_state) {
    +  // save the query to matches
    

    Comments should begin with a capital letter.

ushashree_m’s picture

Hi Albert, Thanks for your suggestions. I have worked on your comments, please check.

Albert Volkman’s picture

  1. +++ b/userpoints.admin.inc
    @@ -373,6 +373,16 @@ function userpoints_admin_txn_submit($form, &$form_state) {
    +function userpoints_admin_points_callback() {
    

    Missing @return

  2. +++ b/userpoints.admin.inc
    @@ -809,3 +817,98 @@ function userpoints_admin_settings($form, &$form_state) {
    +    '#collapsible' => FALSE,
    +    '#collapsed' => FALSE,
    

    These are already defaulted to FALSE, so these are unnecessary.

  3. +++ b/userpoints.admin.inc
    @@ -809,3 +817,98 @@ function userpoints_admin_settings($form, &$form_state) {
    +      '#markup' => l(t('Reset'), $_GET['q'], array('attributes' => array('class' => array('button')))),
    

    If you're going to use a render array, there's no reason to use #markup for a link. Use a link render array.

  4. +++ b/userpoints.admin.inc
    @@ -809,3 +817,98 @@ function userpoints_admin_settings($form, &$form_state) {
    +  if (isset($form_state['values']['user_name']) && !empty($form_state['values']['user_name'])) {
    

    The isset() is redundant, please remove it.

  5. +++ b/userpoints.admin.inc
    @@ -809,3 +817,98 @@ function userpoints_admin_settings($form, &$form_state) {
    +  $form_state['redirect'] = array(
    +    'admin/config/people/userpoints', $queryargs
    +  );
    

    This should all be on the same line.

  6. +++ b/userpoints.admin.inc
    @@ -809,3 +817,98 @@ function userpoints_admin_settings($form, &$form_state) {
    + * @param $search
    

    Missing variable type.

  7. +++ b/userpoints.admin.inc
    @@ -809,3 +817,98 @@ function userpoints_admin_settings($form, &$form_state) {
    +  // Return the result to the form in json.
    

    drupal_json_output() doesn't return, it sends the data to the browser.

  8. +++ b/userpoints.admin.inc
    @@ -809,3 +817,98 @@ function userpoints_admin_settings($form, &$form_state) {
    +function _userpoints_get_users_info($string = '') {
    

    Missing docblock entirely.

ushashree_m’s picture

Hi Albert, I have Worked on given comments, Please check.

Albert Volkman’s picture

  1. +++ b/userpoints.admin.inc
    @@ -373,6 +373,19 @@ function userpoints_admin_txn_submit($form, &$form_state) {
    + * @return
    

    Missing return type (array).

  2. +++ b/userpoints.admin.inc
    @@ -809,3 +820,97 @@ function userpoints_admin_settings($form, &$form_state) {
    + * Menu callback; Retrieve a JSON object containing autocomplete suggestions for users with userpoints.
    + */
    ...
    + * Query to retrieve users with userpoints.
    

    Missing `@param` and `@return`.

  3. +++ b/userpoints.admin.inc
    @@ -809,3 +820,97 @@ function userpoints_admin_settings($form, &$form_state) {
    + * Query to retrieve users with userpoints.
    

    Greater than 80 characters, please shorten. Suggestion: `Menu callback: Retrieve autocomplete suggestions for users with points.`

ushashree_m’s picture

Please find the updated patch.

ushashree_m’s picture

In the Previous patch module file changes are not updated. Please find the Updated Patch.

Albert Volkman’s picture

Status: Active » Reviewed & tested by the community