Moodle
  1. Moodle
  2. MDL-36767

User filtering problems with multiple filters

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.6, 2.3.3, 2.4
    • Fix Version/s: 2.2.7, 2.3.4
    • Component/s: Administration
    • Labels:
    • Testing Instructions:
      Hide

      Test 1:

      Run MDLQA-4823

      Test 2:

      1. Go to browse users
      2. Add 'suspended' filter
      3. Add a 'auth' filter
      4. Click add filter and you should not see any error and check result if it's correct
      5. Add a 'confirmed' filter and you should not see any error
      6. Remove 'suspended' filter and no error should occur.
      7. Try adding filter on bulk user actions page and you should not see any error
      Show
      Test 1: Run MDLQA-4823 Test 2: Go to browse users Add 'suspended' filter Add a 'auth' filter Click add filter and you should not see any error and check result if it's correct Add a 'confirmed' filter and you should not see any error Remove 'suspended' filter and no error should occur. Try adding filter on bulk user actions page and you should not see any error
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull Master Branch:
      wip-mdl-36767
    • Rank:
      46279

      Description

      Discovered while testing http://tracker.moodle.org/browse/MDLQA-4823

      Steps to reproduce:

      1. Go to browse users or to bulk user actions
      2. Add a 'confirmed' or 'suspended' filter
      3. Add a 'auth' filter

      When you press the 'Filter' button an error is thrown.

      Debug info: 
      Error code: invalidqueryparam
      Stack trace:
      line 800 of /lib/dml/moodle_database.php: dml_exception thrown
      line 1018 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->fix_sql_params()
      line 425 of /lib/datalib.php: call to mysqli_native_moodle_database->get_records_sql()
      line 186 of /admin/user.php: call to get_users_listing()
      

      I've also reproduced the problem in latests moodle 23 integration and moodle 22 integration

      Debug info: 
      Error code: invalidqueryparam
      Stack trace:
      line 783 of /lib/dml/moodle_database.php: dml_exception thrown
      line 961 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->fix_sql_params()
      line 318 of /lib/datalib.php: call to mysqli_native_moodle_database->get_records_sql()
      line 186 of /admin/user.php: call to get_users_listing()
      

        Issue Links

          Activity

          Hide
          Rajesh Taneja added a comment -

          user_filter_yesno extends user_filter_simpleselect, so both use same filter name "'ex_simpleselect'.$counter++"
          Unfortunately, the classes are different and hence they both will have a different static count for $counter, which is causing problem.

          It will be nice to retractor code to avoid static usage, but for now I am going with overriding get_sql_filter(), in user_filter_yesno to use 'ex_yesno'.$counter++.
          Which will solve the issue. It might be nice to re-visit this area to have a clean implementation.

          Show
          Rajesh Taneja added a comment - user_filter_yesno extends user_filter_simpleselect, so both use same filter name "'ex_simpleselect'.$counter++" Unfortunately, the classes are different and hence they both will have a different static count for $counter, which is causing problem. It will be nice to retractor code to avoid static usage, but for now I am going with overriding get_sql_filter(), in user_filter_yesno to use 'ex_yesno'.$counter++. Which will solve the issue. It might be nice to re-visit this area to have a clean implementation.
          Hide
          Rossiani Wijaya added a comment -

          Hi Raj,

          The patch works great.

          Some feedback for the patch:

          1. line 30, removed $params
          2. line 33, it should probably returning an array() instead of empty string (as stated on the docs)
          3. line 35, spacing for the array on the second param.

          [y] Syntax
          [y] Output
          [y] Whitespace
          [-] Language
          [-] Databases
          [y] Testing, master only
          [-] Security
          [-] Documentation
          [y] Git
          [y] Sanity check

          Feel free to submit it for integration review.

          Rosie

          Show
          Rossiani Wijaya added a comment - Hi Raj, The patch works great. Some feedback for the patch: line 30, removed $params line 33, it should probably returning an array() instead of empty string (as stated on the docs) line 35, spacing for the array on the second param. [y] Syntax [y] Output [y] Whitespace [-] Language [-] Databases [y] Testing, master only [-] Security [-] Documentation [y] Git [y] Sanity check Feel free to submit it for integration review. Rosie
          Hide
          Aparup Banerjee added a comment -

          hi, any feedback on the peer-review ? please record it in short here

          Show
          Aparup Banerjee added a comment - hi, any feedback on the peer-review ? please record it in short here
          Hide
          Rajesh Taneja added a comment -

          My Bad,

          Forgot to update branches. It's done now, all suggestion were integrated.

          Show
          Rajesh Taneja added a comment - My Bad, Forgot to update branches. It's done now, all suggestion were integrated.
          Hide
          Aparup Banerjee added a comment -

          Thanks, thats been integrated into 22, 23 and master.

          note: i saw in tex.php get_sql_filter() still returns ''

          Show
          Aparup Banerjee added a comment - Thanks, thats been integrated into 22, 23 and master. note: i saw in tex.php get_sql_filter() still returns ''
          Hide
          David Monllaó added a comment -

          It passes. Tested in 22, 23 and master, it works as expected.

          Show
          David Monllaó added a comment - It passes. Tested in 22, 23 and master, it works as expected.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Y E S !

          Closing as fixed, many thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Y E S ! Closing as fixed, many thanks!

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: