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

Add option to Scorm setting to display or hide activity name

    Details

    • Testing Instructions:
      Hide
      1. Create a SCORM activity (set name, description and SCORM file). The new setting, displayactivitytitle, defaults to true.
      2. Verify that when a user runs the activity (player.php) with the displayactivitytitle flag having been set to true, the header displays the title of the activity.
      3. Edit the settings of the activity, and set displayactivitytitle to false.
      4. Verify that when a user runs the activity, the header no longer displays.
      5. Backup the SCORM activity.
      6. Delete the SCORM activity and restore it from backup.
      7. Verify that the displayactivitytitle setting is as it was before the backup.
      Show
      Create a SCORM activity (set name, description and SCORM file). The new setting, displayactivitytitle, defaults to true. Verify that when a user runs the activity (player.php) with the displayactivitytitle flag having been set to true, the header displays the title of the activity. Edit the settings of the activity, and set displayactivitytitle to false. Verify that when a user runs the activity, the header no longer displays. Backup the SCORM activity. Delete the SCORM activity and restore it from backup. Verify that the displayactivitytitle setting is as it was before the backup.
    • Affected Branches:
      MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_27_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-42588-master
    • Sprint:
      FRONTEND Sprint 11
    • Story Points (Obsolete):
      5
    • Sprint:
      FRONTEND Sprint 11

      Description

      From the discussion on MDL-41797, we are allowing to have option to display or hide the activity name.

      Currently, there's no option to do this in Scorm. This option could be used to prevent duplication of heading display on the page as mentioned in discussion also occurs in https://moodle.org/mod/forum/discuss.php?d=242457.

      A draw back for implementing this is the possibility of losing the page hierarchy level for heading. Some user with screen reader or assistive technology use these header levels as page structure to navigate the web page.

      I also created a forum discussion to get feedback from the community https://moodle.org/mod/forum/discuss.php?d=244295.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            danmarsden Dan Marsden added a comment -

            feels a bit like setting overkill to me - but if it's consistent with other modules then I suppose it makes sense. - thanks.

            Show
            danmarsden Dan Marsden added a comment - feels a bit like setting overkill to me - but if it's consistent with other modules then I suppose it makes sense. - thanks.
            Hide
            danmarsden Dan Marsden added a comment -

            I haven't peer reviewed this completely but it mostly looks ok
            one thing to note - the lang string "displayactivityname_help" isn't quite right "..show the display the.."

            thanks!

            Show
            danmarsden Dan Marsden added a comment - I haven't peer reviewed this completely but it mostly looks ok one thing to note - the lang string "displayactivityname_help" isn't quite right "..show the display the.." thanks!
            Hide
            jethac Jetha Chan added a comment -

            Oof, that's embarrassing. Good catch, Dan!

            Show
            jethac Jetha Chan added a comment - Oof, that's embarrassing. Good catch, Dan!
            Hide
            cibot CiBoT added a comment -

            Results for MDL-42588

            • Remote repository: git://github.com/jethac/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-42588 Remote repository: git://github.com/jethac/moodle.git Remote branch MDL-42588 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2325 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2325/artifact/work/smurf.html
            Hide
            cibot CiBoT added a comment -

            Results for MDL-42588

            • Remote repository: git://github.com/jethac/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-42588 Remote repository: git://github.com/jethac/moodle.git Remote branch MDL-42588 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2331 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2331/artifact/work/smurf.html
            Hide
            cibot CiBoT added a comment -

            Results for MDL-42588

            • Remote repository: git://github.com/jethac/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-42588 Remote repository: git://github.com/jethac/moodle.git Remote branch MDL-42588 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2334 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2334/artifact/work/smurf.html
            Hide
            damyon Damyon Wiese added a comment -

            Taking this peer review (Dan has had a look already).

            Show
            damyon Damyon Wiese added a comment - Taking this peer review (Dan has had a look already).
            Hide
            dobedobedoh Andrew Nicols added a comment -

            I was just about to take this for peer review too but you're welcome to it.
            One issue which stands out is that the displayactivityname DB field is using a length of 4 for a boolean value when 1 would suffice.

            Show
            dobedobedoh Andrew Nicols added a comment - I was just about to take this for peer review too but you're welcome to it. One issue which stands out is that the displayactivityname DB field is using a length of 4 for a boolean value when 1 would suffice.
            Hide
            damyon Damyon Wiese added a comment -

            [Y] Syntax
            [Y] Whitespace
            [Y] Output
            [Y] Language
            [-] Databases
            [-] Testing (instructions and automated tests) - The testing instructions are good - maybe this could have been added to the behat tests, but thats probably overkill for such a small feature.
            [-] Security
            [-] Documentation
            [Y] Git
            [-] Third party code
            [Y] Sanity check

            Looks good Jetha - thanks.

            Submitting this for integration.

            Show
            damyon Damyon Wiese added a comment - [Y] Syntax [Y] Whitespace [Y] Output [Y] Language [-] Databases [-] Testing (instructions and automated tests) - The testing instructions are good - maybe this could have been added to the behat tests, but thats probably overkill for such a small feature. [-] Security [-] Documentation [Y] Git [-] Third party code [Y] Sanity check Looks good Jetha - thanks. Submitting this for integration.
            Hide
            damyon Damyon Wiese added a comment -

            Noting Andrews comment - but 4 is correct according to: http://docs.moodle.org/dev/Database

            Show
            damyon Damyon Wiese added a comment - Noting Andrews comment - but 4 is correct according to: http://docs.moodle.org/dev/Database
            Hide
            danmarsden Dan Marsden added a comment -

            Thanks Damyon!
            Jetha - you don't need to assign me as peer reviewer on SCORM stuff and I could end up being the bottleneck if you do! this week has been pretty busy so I haven't managed to get the time to do it properly - great that Damyon was able to grab it.

            Show
            danmarsden Dan Marsden added a comment - Thanks Damyon! Jetha - you don't need to assign me as peer reviewer on SCORM stuff and I could end up being the bottleneck if you do! this week has been pretty busy so I haven't managed to get the time to do it properly - great that Damyon was able to grab it.
            Hide
            cibot CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            marina Marina Glancy added a comment -

            Hi Jetha, thanks for working on it.

            You set the default value in DB = 1, so no upgrade script is required for existing scorms. That's ok. All scorms created before upgrade will be displayed as before.
            But there are several things that I do not like:
            -The default value when creating new scorm activity is 0 - this is change of functionality.

            • try scenario: before upgrade create a course with scorm and back it up. Upgrade. Restore the course in the new course. You will see that in the first (original) course the name is displayed, in the second (restored) course the name is not displayed. This happens because there is no default value in restore process if the field 'displayactivityname' is not in the backup file.
            • And at last the help text should say something like 'Whether or not to display the activity name above the player.' On all other screens activity name is displayed regardless of the setting but it's not clear from the help text.

            If you can fix those points before the end of the day, I'll integrate it in this cycle. Thanks again.

            Show
            marina Marina Glancy added a comment - Hi Jetha, thanks for working on it. You set the default value in DB = 1, so no upgrade script is required for existing scorms. That's ok. All scorms created before upgrade will be displayed as before. But there are several things that I do not like: -The default value when creating new scorm activity is 0 - this is change of functionality. try scenario: before upgrade create a course with scorm and back it up. Upgrade. Restore the course in the new course. You will see that in the first (original) course the name is displayed, in the second (restored) course the name is not displayed. This happens because there is no default value in restore process if the field 'displayactivityname' is not in the backup file. And at last the help text should say something like 'Whether or not to display the activity name above the player .' On all other screens activity name is displayed regardless of the setting but it's not clear from the help text. If you can fix those points before the end of the day, I'll integrate it in this cycle. Thanks again.
            Hide
            marina Marina Glancy added a comment -

            just now reviewing your issue MDL-42575 I noticed that you might want to add the same default setting in scorm generator

            Show
            marina Marina Glancy added a comment - just now reviewing your issue MDL-42575 I noticed that you might want to add the same default setting in scorm generator
            Hide
            jethac Jetha Chan added a comment -

            Hi Marina,
            I'll get right on those. Thanks!

            Show
            jethac Jetha Chan added a comment - Hi Marina, I'll get right on those. Thanks!
            Hide
            marina Marina Glancy added a comment -

            Thanks Jetha, integrated in master

            Show
            marina Marina Glancy added a comment - Thanks Jetha, integrated in master
            Hide
            jethac Jetha Chan added a comment -

            Re: the feedback you gave me:

            • I've added an admin setting similar to MDL-42575's that'll control the default value when creating new SCORM activities (it defaults to being set to true).
            • I've added a !isset check in process_scorm($data) (mod/scorm/backup/moodle2/restore_scorm_stepslib.php:56) that adds a default value for displaying the title in the restore process if there isn't a value in the file being restored from, effectively dealing with the scenario you posed (tested with a SCORM module on pre-patch MOODLE_25_MASTER, restored onto post-patch master without difficulty).
            • I've amended the help text to say "Whether or not to display the activity name above the player."

            Thanks so much for your feedback!

            Show
            jethac Jetha Chan added a comment - Re: the feedback you gave me: I've added an admin setting similar to MDL-42575 's that'll control the default value when creating new SCORM activities (it defaults to being set to true). I've added a !isset check in process_scorm($data) (mod/scorm/backup/moodle2/restore_scorm_stepslib.php:56) that adds a default value for displaying the title in the restore process if there isn't a value in the file being restored from, effectively dealing with the scenario you posed (tested with a SCORM module on pre-patch MOODLE_25_MASTER, restored onto post-patch master without difficulty). I've amended the help text to say "Whether or not to display the activity name above the player." Thanks so much for your feedback!
            Hide
            marina Marina Glancy added a comment -

            Jetha, you replaced $plugin->version with $modle->version in version.php and it triggered error in integration server. I have added commit to change it back.
            See MDL-43040

            Show
            marina Marina Glancy added a comment - Jetha, you replaced $plugin->version with $modle->version in version.php and it triggered error in integration server. I have added commit to change it back. See MDL-43040
            Hide
            jethac Jetha Chan added a comment -

            I'm honestly not sure how that happened. Sorry to make you do that! I'll be more careful in future.

            Show
            jethac Jetha Chan added a comment - I'm honestly not sure how that happened. Sorry to make you do that! I'll be more careful in future.
            Hide
            danmarsden Dan Marsden added a comment -

            I just realised there's a problem with the heading in player.php - it shouldn't show at all when in popup mode.

            it would be good if we could modify this patch so that the heading doesn't show at all in popup mode and the setting is disabled when the option "new window" is set.

            I'm ok with this going into integration in master only as it stands - we can sort out the issues next week in MDL-44738

            Show
            danmarsden Dan Marsden added a comment - I just realised there's a problem with the heading in player.php - it shouldn't show at all when in popup mode. it would be good if we could modify this patch so that the heading doesn't show at all in popup mode and the setting is disabled when the option "new window" is set. I'm ok with this going into integration in master only as it stands - we can sort out the issues next week in MDL-44738
            Hide
            skodak Petr Skoda added a comment -

            When adding new SCORM activity the title is now disabled by default, that looks like a major change to me. I expected there would be a new default setting in "Other default settings" section.

            Show
            skodak Petr Skoda added a comment - When adding new SCORM activity the title is now disabled by default, that looks like a major change to me. I expected there would be a new default setting in "Other default settings" section.
            Hide
            skodak Petr Skoda added a comment - - edited

            failing for now, I think we should add something like this to mod_form.php

                    // Display activity name.
                    $mform->addElement('advcheckbox', 'displayactivityname', get_string('displayactivityname', 'scorm'));
                    $mform->addHelpButton('displayactivityname', 'displayactivityname', 'scorm');
                    $mform->setDefault('displayactivityname', $cfgscorm->displayactivityname);
            

            and settings.php

                $settings->add(new admin_setting_heading('scorm/othersettings', get_string('defaultothersettings', 'scorm'), ''));
             
                $settings->add(new admin_setting_configcheckbox('scorm/displayactivityname',
                    get_string('displayactivityname', 'scorm'), '', '1'));
            

            Show
            skodak Petr Skoda added a comment - - edited failing for now, I think we should add something like this to mod_form.php // Display activity name. $mform->addElement('advcheckbox', 'displayactivityname', get_string('displayactivityname', 'scorm')); $mform->addHelpButton('displayactivityname', 'displayactivityname', 'scorm'); $mform->setDefault('displayactivityname', $cfgscorm->displayactivityname); and settings.php $settings->add(new admin_setting_heading('scorm/othersettings', get_string('defaultothersettings', 'scorm'), ''));   $settings->add(new admin_setting_configcheckbox('scorm/displayactivityname', get_string('displayactivityname', 'scorm'), '', '1'));
            Show
            skodak Petr Skoda added a comment - https://github.com/skodak/moodle/commit/fd1ead81f6cc98b1ef0d3abe0f4781b0d6a94d04
            Hide
            marina Marina Glancy added a comment -

            this is strange, I commented the same during integration review and I thought Jetha set the default to 1. Unless the commit got lost somehow

            Show
            marina Marina Glancy added a comment - this is strange, I commented the same during integration review and I thought Jetha set the default to 1. Unless the commit got lost somehow
            Hide
            skodak Petr Skoda added a comment -

            yes, the database default was set to 1, but that is used in upgrades only, the forms need the defaults too

            Show
            skodak Petr Skoda added a comment - yes, the database default was set to 1, but that is used in upgrades only, the forms need the defaults too
            Hide
            marina Marina Glancy added a comment -

            it looks like I integrated some other branch.... because I see this code in the diff link above:
            https://github.com/jethac/moodle/commit/74d2a9df6d1a17772ec3949b99c20a31b41aaa9f#diff-79cedf5a4a4fcc02a64746b7321d52bbR35

            Show
            marina Marina Glancy added a comment - it looks like I integrated some other branch.... because I see this code in the diff link above: https://github.com/jethac/moodle/commit/74d2a9df6d1a17772ec3949b99c20a31b41aaa9f#diff-79cedf5a4a4fcc02a64746b7321d52bbR35
            Hide
            skodak Petr Skoda added a comment -

            I do not understand, this is my proposed commit on top of current integration https://github.com/skodak/moodle/compare/e1190da...scorm

            Show
            skodak Petr Skoda added a comment - I do not understand, this is my proposed commit on top of current integration https://github.com/skodak/moodle/compare/e1190da...scorm
            Hide
            marina Marina Glancy added a comment -

            Petr, it looks like it should be in "Default display settings" where Jetha put it and not in "Other default settings". Besides it needs a version bump, otherwise the default does not apply

            Show
            marina Marina Glancy added a comment - Petr, it looks like it should be in "Default display settings" where Jetha put it and not in "Other default settings". Besides it needs a version bump, otherwise the default does not apply
            Hide
            jethac Jetha Chan added a comment -

            I think this might be because I didn't set this issue back in development while I was working on those fixes yesterday. I'm really sorry for any confusion caused!

            Show
            jethac Jetha Chan added a comment - I think this might be because I didn't set this issue back in development while I was working on those fixes yesterday. I'm really sorry for any confusion caused!
            Hide
            skodak Petr Skoda added a comment -

            Yep, version bump should be done, but it is not critical if it was already done this week, and yes it should be in different settings sections (I missed that, sorry)

            Show
            skodak Petr Skoda added a comment - Yep, version bump should be done, but it is not critical if it was already done this week, and yes it should be in different settings sections (I missed that, sorry)
            Hide
            marina Marina Glancy added a comment - - edited

            To Jetha:
            no you should not change the status when the issue is in integration. I don't know how it happened, I might have picked up the branch before you actually pushed it. Don't worry, let's just fix it now

            Show
            marina Marina Glancy added a comment - - edited To Jetha: no you should not change the status when the issue is in integration. I don't know how it happened, I might have picked up the branch before you actually pushed it. Don't worry, let's just fix it now
            Hide
            marina Marina Glancy added a comment -

            pulled fix from Jetha, back for testing
            Thanks!

            Show
            marina Marina Glancy added a comment - pulled fix from Jetha, back for testing Thanks!
            Hide
            skodak Petr Skoda added a comment -

            works fine now for me, thanks!

            Show
            skodak Petr Skoda added a comment - works fine now for me, thanks!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Clothes and manners do
            not make the man; but,
            when he is made, they
            greatly improve his appearance.

            ---- Henry Ward Beecher

            What a week, your changes are now part of Moodle, well done!

            Closing, thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Clothes and manners do not make the man; but, when he is made, they greatly improve his appearance. ---- Henry Ward Beecher What a week, your changes are now part of Moodle, well done! Closing, thanks!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  12/May/14

                  Agile