Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-36767

User filtering problems with multiple filters

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

      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()

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              rajeshtaneja 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
              rajeshtaneja 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
              rwijaya 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
              rwijaya 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
              nebgor Aparup Banerjee added a comment -

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

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

              My Bad,

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

              Show
              rajeshtaneja Rajesh Taneja added a comment - My Bad, Forgot to update branches. It's done now, all suggestion were integrated.
              Hide
              nebgor 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
              nebgor 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
              dmonllao David Monllaó added a comment -

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

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

              Y E S !

              Closing as fixed, many thanks!

              Show
              stronk7 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:
                    Fix Release Date:
                    14/Jan/13