Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Book
    • Labels:
    • Testing Instructions:
      Hide

      There's no visible changes on the front-end.

      #. add book resource
      #. create some chapters
      #. test the functionality of the book module and make sure nothing is broken.

      Show
      There's no visible changes on the front-end. #. add book resource #. create some chapters #. test the functionality of the book module and make sure nothing is broken.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      40386

      Description

      During my integration review I noted several things about the new book module which require thought/fixing/clarification.
      As none of them were blockers it was integrated however we should still have a look over this list and act upon each point accordingly:

      Requiring fixes/changes:

      • README.md needs updating to reflect its integration into core. Not urgent but before release probably (defintely "no more developement planned" ).
      • The opening line of the boilerplate for each file is inconsistent. Some say part of Moodle, others Book module for Moodle, and others again Book plugin for Moodle.
      • $PAGE->set_title calls format_string internally (aweful) but mean no need to wrap things in format_string when setting page title[several files affected].
      • $PAGE->set_heading is the same as above.
      • booktool_***_extend_settings_navigation no need to check modname !== book that has happened within the calling function book_extend_settings_navigation. (Lots of unused globals in those functions as well).
      • book_preload_chapters: next and prev values are never set correctly. Not a bug as they are also not used presently (but should be fixed or removed none the less).
      • tool/exportimscp/index.php unused lang string vars.

      Requiring clarification:

      • I couldn't find or determine a use for book_log function in locallib.php
      • PARAM_RAW for the chapter title, is that correct will we allow HTML + friends there?
      • Is there a need to add mod_book as a body class. You can be sure the body class path-mod-book has been added for all pages within the book module.
      • I couldn't find the use of the chapterscount string. If it is used should that have a {$a}.

      Improvements, usability, accessibility, ideas:

      • It would be nice if when editing the first chapter (such as when you've just created the activity) if the would freeze the "subchapter" option and add a note about it being frozen because the first page cannot be a subchapter. Just a smidge better than ignoring it for friendliness.
      • book_get_toc would be great to convert output to use either a renderer preferably or at least standard output mechanics.
      • Simple observation renderers arn't being used, it would be great if we could convert things to make use of a renderer, simple as it may be.

        Activity

        Hide
        Sam Hemelryk added a comment -

        Hi Petr,

        Have assigned this to you so that you can have a look over the points I have made if you have time. Otherwise perhaps some ideas and re-assign to someone else to get things acted upon where necessary.
        Some tasks represent improvements that should be turned into separate issues to be looked at some time in the future, however having them all here forces us to make a decision as I will mark this "Must fix for 2.3" so that we can be sure things are at least looked at before 2.3.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Petr, Have assigned this to you so that you can have a look over the points I have made if you have time. Otherwise perhaps some ideas and re-assign to someone else to get things acted upon where necessary. Some tasks represent improvements that should be turned into separate issues to be looked at some time in the future, however having them all here forces us to make a decision as I will mark this "Must fix for 2.3" so that we can be sure things are at least looked at before 2.3. Cheers Sam
        Hide
        Petr Škoda added a comment -

        I never installed book on any 2.x production site and nobody wanted to fund code upgrade so I never cleaned up the codebase properly to new coding style...

        Fixes:

        • yes, README needs to be fixed
        • yes, boilerplates should be consistent
        • yes, set_title() mess dates back to pre-navigation times, needs to be fixed
        • ditto for set_heading
        • yes, for navigation mess
        • yes, preload should be fixed
        • Eloy implemented the original imscp functionality, right?

        Clarification:

        • I do not remember what the logging was supposed to do
        • HTML was everywhere in the old days, some ppl use colours in title chapters
        • this dates back to disagreement if we should use proper frankenstyle for plugin classes (exactly like blocks do it now) or rely on directory and file names in CSS classes - feel free to change it in any way
        • chapter count might have been in print view, I do not remember

        Improvements:

        • freezing of subchapter - good idea!
        • yes, output renderers should be used

        I was wondering who is going to maintain book now, was it already decided?

        Show
        Petr Škoda added a comment - I never installed book on any 2.x production site and nobody wanted to fund code upgrade so I never cleaned up the codebase properly to new coding style... Fixes: yes, README needs to be fixed yes, boilerplates should be consistent yes, set_title() mess dates back to pre-navigation times, needs to be fixed ditto for set_heading yes, for navigation mess yes, preload should be fixed Eloy implemented the original imscp functionality, right? Clarification: I do not remember what the logging was supposed to do HTML was everywhere in the old days, some ppl use colours in title chapters this dates back to disagreement if we should use proper frankenstyle for plugin classes (exactly like blocks do it now) or rely on directory and file names in CSS classes - feel free to change it in any way chapter count might have been in print view, I do not remember Improvements: freezing of subchapter - good idea! yes, output renderers should be used I was wondering who is going to maintain book now, was it already decided?
        Hide
        Tõnis Tartes added a comment - - edited

        I made some changes to the book module and added a patch, described here.

        Requiring fixes/changes:

        Skipped this for the moment - README.md needs updating to reflect its integration into core. Not urgent but before release probably (defintely "no more developement planned" ).
        Fixed - The opening line of the boilerplate for each file is inconsistent. Some say part of Moodle, others Book module for Moodle, and others again Book plugin for Moodle.
        Fixed - $PAGE->set_title calls format_string internally (aweful) but mean no need to wrap things in format_string when setting page title[several files affected].
        Fixed - $PAGE->set_heading is the same as above.
        Fixed - booktool_***_extend_settings_navigation no need to check modname !== book that has happened within the calling function book_extend_settings_navigation. (Lots of unused globals in those functions as well).
        I was unsure about this one - book_preload_chapters: next and prev values are never set correctly. Not a bug as they are also not used presently (but should be fixed or removed none the less).
        Removed these - tool/exportimscp/index.php unused lang string vars.

        Requiring clarification:

        Skipped - I couldn't find or determine a use for book_log function in locallib.php
        Skipped, because sometimes titles are coloured - PARAM_RAW for the chapter title, is that correct will we allow HTML + friends there?
        Removed the extra class - Is there a need to add mod_book as a body class. You can be sure the body class path-mod-book has been added for all pages within the book module.
        Removed it - I couldn't find the use of the chapterscount string. If it is used should that have a {$a}.

        Improvements, usability, accessibility, ideas:

        Fixed - It would be nice if when editing the first chapter (such as when you've just created the activity) if the would freeze the "subchapter" option and add a note about it being frozen because the first page cannot be a subchapter. Just a smidge better than ignoring it for friendliness.
        Fixed - book_get_toc would be great to convert output to use either a renderer preferably or at least standard output mechanics.
        Simple observation renderers arn't being used, it would be great if we could convert things to make use of a renderer, simple as it may be.

        I hope my patch helps a little..

        Show
        Tõnis Tartes added a comment - - edited I made some changes to the book module and added a patch, described here. Requiring fixes/changes: Skipped this for the moment - README.md needs updating to reflect its integration into core. Not urgent but before release probably (defintely "no more developement planned" ). Fixed - The opening line of the boilerplate for each file is inconsistent. Some say part of Moodle, others Book module for Moodle, and others again Book plugin for Moodle. Fixed - $PAGE->set_title calls format_string internally (aweful) but mean no need to wrap things in format_string when setting page title [several files affected] . Fixed - $PAGE->set_heading is the same as above. Fixed - booktool_***_extend_settings_navigation no need to check modname !== book that has happened within the calling function book_extend_settings_navigation. (Lots of unused globals in those functions as well). I was unsure about this one - book_preload_chapters: next and prev values are never set correctly. Not a bug as they are also not used presently (but should be fixed or removed none the less). Removed these - tool/exportimscp/index.php unused lang string vars. Requiring clarification: Skipped - I couldn't find or determine a use for book_log function in locallib.php Skipped, because sometimes titles are coloured - PARAM_RAW for the chapter title, is that correct will we allow HTML + friends there? Removed the extra class - Is there a need to add mod_book as a body class. You can be sure the body class path-mod-book has been added for all pages within the book module. Removed it - I couldn't find the use of the chapterscount string. If it is used should that have a {$a}. Improvements, usability, accessibility, ideas: Fixed - It would be nice if when editing the first chapter (such as when you've just created the activity) if the would freeze the "subchapter" option and add a note about it being frozen because the first page cannot be a subchapter. Just a smidge better than ignoring it for friendliness. Fixed - book_get_toc would be great to convert output to use either a renderer preferably or at least standard output mechanics. Simple observation renderers arn't being used, it would be great if we could convert things to make use of a renderer, simple as it may be. I hope my patch helps a little..
        Hide
        Rossiani Wijaya added a comment -

        Hi Tonis,

        Thank you for proving patch to improve the book module.

        Could you please rebase/update the patch?

        Rosie

        Show
        Rossiani Wijaya added a comment - Hi Tonis, Thank you for proving patch to improve the book module. Could you please rebase/update the patch? Rosie
        Hide
        Rossiani Wijaya added a comment -

        Hi Sam,

        Could you peer review this?

        Thanks.

        Show
        Rossiani Wijaya added a comment - Hi Sam, Could you peer review this? Thanks.
        Hide
        Sam Hemelryk added a comment -

        Hi Rossie,

        Changes look real good thanks and everything in the list has been addressed.
        The following are the notes I made going through. All pretty minor I think. The global $CFG change perhaps the only slightly more requiring note.

        1. Empty line at start of function book_extend_settings_navigation should be removed. Also additional new line that has been added there not needed.
        2. Same for booktool_***_extend_settings_navigation functions.
        3. book_preload_chapters next/prev are not used outside of that function. I think better to remove them rather than have them set. One less area for regressions in the future.
        4. edit_form.php line 40 (the if statement) needs more work. hehe look and you will see what I mean.
        5. edit_form.php should use hardFreeze rather than the disabled attribute.
        6. I don't like the removal of the `global $CFG`'s from lib.php where includes are occuring. Also the addition of global $CFG to counter that change in locallib.php. Certainly as it was before those changes is better, espcially in the case of tool includes, as although we may not have any doing requires at the moment someone one day may, and its easier for them to have what they need than to have to work it out. Will help consistency in code as well. My +1 to revert the global CFG changes.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Rossie, Changes look real good thanks and everything in the list has been addressed. The following are the notes I made going through. All pretty minor I think. The global $CFG change perhaps the only slightly more requiring note. Empty line at start of function book_extend_settings_navigation should be removed. Also additional new line that has been added there not needed. Same for booktool_***_extend_settings_navigation functions. book_preload_chapters next/prev are not used outside of that function. I think better to remove them rather than have them set. One less area for regressions in the future. edit_form.php line 40 (the if statement) needs more work. hehe look and you will see what I mean. edit_form.php should use hardFreeze rather than the disabled attribute. I don't like the removal of the `global $CFG`'s from lib.php where includes are occuring. Also the addition of global $CFG to counter that change in locallib.php. Certainly as it was before those changes is better, espcially in the case of tool includes, as although we may not have any doing requires at the moment someone one day may, and its easier for them to have what they need than to have to work it out. Will help consistency in code as well. My +1 to revert the global CFG changes. Cheers Sam
        Hide
        Rossiani Wijaya added a comment -

        Hi Sam,

        I just updated the patch. Could you take a look the patch again?

        Thanks
        Rosie

        Show
        Rossiani Wijaya added a comment - Hi Sam, I just updated the patch. Could you take a look the patch again? Thanks Rosie
        Hide
        Ankit Agarwal added a comment -

        Rosie asked me to have a look at the patch and here is my review:-

        1. $mform->hardFreeze('subchapter'); is done always , no matter if it is the first chapter or not
        2. AFAIK we are not allowed to create conditional elements in a mform definition (http://docs.moodle.org/dev/lib/formslib.php_Form_Definition)
        3. Line 53 and 54 are empty lines with white spaces in edit_form.php

        Thanks
        Ankit

        Show
        Ankit Agarwal added a comment - Rosie asked me to have a look at the patch and here is my review:- $mform->hardFreeze('subchapter'); is done always , no matter if it is the first chapter or not AFAIK we are not allowed to create conditional elements in a mform definition ( http://docs.moodle.org/dev/lib/formslib.php_Form_Definition ) Line 53 and 54 are empty lines with white spaces in edit_form.php Thanks Ankit
        Hide
        Rossiani Wijaya added a comment -

        Thank Ankit.

        1. fixed
        2. I kept the disabled message with the definition(), because it can't be overwrite it within the definition_after_data().
        3. fixed

        Updating the patch to your suggestion.

        Show
        Rossiani Wijaya added a comment - Thank Ankit. 1. fixed 2. I kept the disabled message with the definition(), because it can't be overwrite it within the definition_after_data(). 3. fixed Updating the patch to your suggestion.
        Hide
        Rossiani Wijaya added a comment -

        Sending for integration review.

        Show
        Rossiani Wijaya added a comment - Sending for integration review.
        Hide
        Dan Poltawski added a comment -

        Thanks Rosie and Tõnis!

        I've integrated it now. I added one commit doing a slight tweak to comment style to match coding style. I also wasn't keen on the string and discussed with Martin and we changed that.

        Show
        Dan Poltawski added a comment - Thanks Rosie and Tõnis! I've integrated it now. I added one commit doing a slight tweak to comment style to match coding style. I also wasn't keen on the string and discussed with Martin and we changed that.
        Hide
        Tim Barker added a comment -

        Regression tested Book. I noticed that in the Table of contents block if the chapter title is a single word that is incredibly long; the text overlaps the block border.

        This could be a problem in languages such as German where individual words can be concatenated into huge composite words. e.g. Hoechsgeschwindigkeitsbegrenzung is maximum speed limit in German.

        Show
        Tim Barker added a comment - Regression tested Book. I noticed that in the Table of contents block if the chapter title is a single word that is incredibly long; the text overlaps the block border. This could be a problem in languages such as German where individual words can be concatenated into huge composite words. e.g. Hoechsgeschwindigkeitsbegrenzung is maximum speed limit in German.
        Hide
        Tim Barker added a comment - - edited

        Hoechsgeschwindigkeitsbegrenzung does indeed overlap the Table of contents block if it is a chapter or sub chapter title. This could be a problem in Germany or in a course to teach people German.

        Show
        Tim Barker added a comment - - edited Hoechsgeschwindigkeitsbegrenzung does indeed overlap the Table of contents block if it is a chapter or sub chapter title. This could be a problem in Germany or in a course to teach people German.
        Hide
        Rossiani Wijaya added a comment -

        Tim,

        I created this issue earlier to fix the chapter or sub-chapter title. MDL-33838.

        Show
        Rossiani Wijaya added a comment - Tim, I created this issue earlier to fix the chapter or sub-chapter title. MDL-33838 .
        Hide
        Dan Poltawski added a comment -

        Looks existing issue to me Tim - now we have a new issue can this be passed?

        Show
        Dan Poltawski added a comment - Looks existing issue to me Tim - now we have a new issue can this be passed?
        Hide
        Tim Barker added a comment -

        Now the issue with text wrapping has been mitigated, I am happy to say, it's all OK

        Show
        Tim Barker added a comment - Now the issue with text wrapping has been mitigated, I am happy to say, it's all OK
        Hide
        Eloy Lafuente (stronk7) added a comment -

        And this has been incorporated to all the weekly builds and also, to Moodle 2.3 Release Candidate 1, yay!

        Many, many thanks for your hard work!

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - And this has been incorporated to all the weekly builds and also, to Moodle 2.3 Release Candidate 1, yay! Many, many thanks for your hard work! Ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: