Moodle
  1. Moodle
  2. MDL-28588

Remove IN from sql statments in Basic report plugin

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.2.1
    • Component/s: SCORM
    • Labels:
    • Rank:
      16939

      Description

      In mod/scorm/report/basic/report.php
      SQL IN statements are used as mentioned below:-
      $where = ' WHERE u.id IN (' .$allowedlist. ') AND st.userid IS NOT NULL';
      $where = ' WHERE u.id IN (' .$allowedlist. ') AND st.userid IS NULL';
      $where = ' WHERE u.id IN (' .$allowedlist. ') AND (st.userid IS NOT NULL OR st.userid IS NULL)';

      as per new moodle standards no sql statements should use IN directly in this way
      Reference: http://docs.moodle.org/dev/DB_layer_2.0_migration_docs#The_golden_changes (G7)

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          I just saw this, so will peer review, if that is OK.

          Show
          Tim Hunt added a comment - I just saw this, so will peer review, if that is OK.
          Hide
          Tim Hunt added a comment -

          Changes look good to me. I think this can be integrated. (I have not tested thought.)

          Show
          Tim Hunt added a comment - Changes look good to me. I think this can be integrated. (I have not tested thought.)
          Hide
          Dan Marsden added a comment -

          Hi Ankit - can you please include the same style patch you have in MDL-30440 into this patch?

          Show
          Dan Marsden added a comment - Hi Ankit - can you please include the same style patch you have in MDL-30440 into this patch?
          Hide
          Ankit Agarwal added a comment -

          Thanks Guys I have made the changes and pushing to integration

          @integrators
          Master only as this is an improvement not a bug.
          Thanks

          Show
          Ankit Agarwal added a comment - Thanks Guys I have made the changes and pushing to integration @integrators Master only as this is an improvement not a bug. Thanks
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          Sending this back at the moment as there is a nasty wee regression hidden because things are broken at the moment. Technically it's not possible to reproduce it at the moment but surely it is worth tiding it up now rather than creating a separate bug to fix things later on.

          First up just pointing out that group mode doesn't work for the basic report presently. In order to produce this failure you'll need to fix that (just search for where $currentgroup is being set and you'll see what I mean).
          Once fixed you select a group you'll see there is an exception thrown because of $allowedlist = ($groupstudents); missing the array_keys call.

          Other than that things look spot on.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, Sending this back at the moment as there is a nasty wee regression hidden because things are broken at the moment. Technically it's not possible to reproduce it at the moment but surely it is worth tiding it up now rather than creating a separate bug to fix things later on. First up just pointing out that group mode doesn't work for the basic report presently. In order to produce this failure you'll need to fix that (just search for where $currentgroup is being set and you'll see what I mean). Once fixed you select a group you'll see there is an exception thrown because of $allowedlist = ($groupstudents); missing the array_keys call. Other than that things look spot on. Cheers Sam
          Hide
          Dan Marsden added a comment -

          thanks Sam - we have another open bug for that group stuff - haven't got round to looking at it yet - no one else had noticed yet - Ankit any chance you have time to look? - I'm pretty busy this week!

          Show
          Dan Marsden added a comment - thanks Sam - we have another open bug for that group stuff - haven't got round to looking at it yet - no one else had noticed yet - Ankit any chance you have time to look? - I'm pretty busy this week!
          Hide
          Ankit Agarwal added a comment -

          Hi,
          Linking the issue discussed in the context.
          I will try to get my hands on it asap.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi, Linking the issue discussed in the context. I will try to get my hands on it asap. Thanks
          Hide
          Ankit Agarwal added a comment -

          @integrators
          This must come together with MDL-28282
          Thanks

          Show
          Ankit Agarwal added a comment - @integrators This must come together with MDL-28282 Thanks
          Hide
          Sam Hemelryk added a comment -

          Thanks guys, this has been integrated now at the same time as MDL-28282

          Show
          Sam Hemelryk added a comment - Thanks guys, this has been integrated now at the same time as MDL-28282
          Hide
          Rajesh Taneja added a comment -

          No change in functionality.
          Thanks Ankit, for fixing this

          Show
          Rajesh Taneja added a comment - No change in functionality. Thanks Ankit, for fixing this
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yes, you did it!

          Now your code is part of the best weeklies released ever, many thanks!

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Yes, you did it! Now your code is part of the best weeklies released ever, many thanks! Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: