Moodle
  1. Moodle
  2. MDL-30965

unused variable table->head in scorm reports

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.1.5, 2.2.2
    • Component/s: SCORM
    • Labels:
    • Testing Instructions:
      Hide

      I can not really think of anything that this change can break. If you want to be extra sure just browse around SCORM reports and make sure there are no regressions

      Show
      I can not really think of anything that this change can break. If you want to be extra sure just browse around SCORM reports and make sure there are no regressions
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      MDL-30965-master
    • Rank:
      37369

      Description

      $table->head is never used in scorm reports

      $table->head[]= format_string($sco->title,'',$formattextoptions);
      

      and in interactions

      $table->head[]= format_string($sco->title);
      

        Activity

        Hide
        Dan Marsden added a comment -

        assigning back to Ankit - feel free to fix this sometime

        Show
        Dan Marsden added a comment - assigning back to Ankit - feel free to fix this sometime
        Hide
        Ankit Agarwal added a comment -

        It looks like this variable was carried over from some historic version. In the current version it has no use, hence am removing those from the code.
        Thanks

        Show
        Ankit Agarwal added a comment - It looks like this variable was carried over from some historic version. In the current version it has no use, hence am removing those from the code. Thanks
        Hide
        Dan Marsden added a comment -

        +1 from me.

        Show
        Dan Marsden added a comment - +1 from me.
        Hide
        Ankit Agarwal added a comment -

        Thanks Dan for the review.
        Sending to integration.

        Show
        Ankit Agarwal added a comment - Thanks Dan for the review. Sending to integration.
        Hide
        Sam Hemelryk added a comment -

        Added missing pull url

        Show
        Sam Hemelryk added a comment - Added missing pull url
        Hide
        Sam Hemelryk added a comment -

        Thanks Ankit, this has been integrated now

        Show
        Sam Hemelryk added a comment - Thanks Ankit, this has been integrated now
        Hide
        Ankit Agarwal added a comment -

        Thanks Sam for fixing the url

        Show
        Ankit Agarwal added a comment - Thanks Sam for fixing the url
        Hide
        Adrian Greeve added a comment -

        I went through and tested with Master, 2.2 and 2.1. I couldn't find any problems related to this fix, though I did find another issue >_>
        This test passes

        Show
        Adrian Greeve added a comment - I went through and tested with Master, 2.2 and 2.1. I couldn't find any problems related to this fix, though I did find another issue >_> This test passes
        Hide
        Ankit Agarwal added a comment -

        Thanks Adrian for the tests.
        If there isn't a MDL for the issue you found already, please create one and link to this issue.
        Thanks

        Show
        Ankit Agarwal added a comment - Thanks Adrian for the tests. If there isn't a MDL for the issue you found already, please create one and link to this issue. Thanks
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Some changes to Moodle should be milestones in the project by themselves.

        This is not the case and your fix is not so important, but your collaboration is highly appreciated, thanks!

        Closing as fixed, ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Some changes to Moodle should be milestones in the project by themselves. This is not the case and your fix is not so important, but your collaboration is highly appreciated, thanks! Closing as fixed, ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: