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

Remove IN from sql statments in Basic report plugin

    Details

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

      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)

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            timhunt Tim Hunt added a comment -

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

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

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

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

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

            Show
            danmarsden 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_frenz 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_frenz 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
            samhemelryk 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
            samhemelryk 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
            danmarsden 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
            danmarsden 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_frenz 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_frenz 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_frenz Ankit Agarwal added a comment -

            @integrators
            This must come together with MDL-28282
            Thanks

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

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

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

            No change in functionality.
            Thanks Ankit, for fixing this

            Show
            rajeshtaneja Rajesh Taneja added a comment - No change in functionality. Thanks Ankit, for fixing this
            Hide
            stronk7 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
            stronk7 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:
                  Fix Release Date:
                  9/Jan/12