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

Non-default Section name not shown at the site level

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.1, 2.6.1, 2.7.1, 2.8, 2.9
    • Fix Version/s: 2.7.4, 2.8.2
    • Component/s: Course
    • Testing Instructions:
      Hide
      1. Go to Admin > Front page > Front page settings
      2. tick 'include topic section'
      3. Go to top-level of a moodle site and log in as admin
      4. Turn editing on
      5. Edit summary for the front page (where section 1 should appear)
      6. Uncheck "Use default section name"
      7. Insert "My site title" into the "Section name" field, and Save changes
      8. Viewing the front page again, the text "My site title" SHOULD appear as a title at the top of the summary.
      9. Edit the section summary again
      10. Check "Use default section name" and save changes
      11. Viewing the front page again, you SHOULD NOT see the section name
      Show
      Go to Admin > Front page > Front page settings tick 'include topic section' Go to top-level of a moodle site and log in as admin Turn editing on Edit summary for the front page (where section 1 should appear) Uncheck "Use default section name" Insert "My site title" into the "Section name" field, and Save changes Viewing the front page again, the text "My site title" SHOULD appear as a title at the top of the summary. Edit the section summary again Check "Use default section name" and save changes Viewing the front page again, you SHOULD NOT see the section name
    • Workaround:
      Hide

      Apply attached patch

      Show
      Apply attached patch
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_26_STABLE, MOODLE_27_STABLE, MOODLE_28_STABLE, MOODLE_29_STABLE
    • Fixed Branches:
      MOODLE_27_STABLE, MOODLE_28_STABLE
    • Pull 2.7 Branch:
    • Pull 2.8 Branch:
    • Pull Master Branch:
      MDL-31822_master
    • Sprint:
      BACKEND Sprint 19
    • Sprint:
      BACKEND Sprint 19
    • Issue size:
      Small

      Description

      Editing the Section summary for a Moodle site, unchecking "Use default section name", and adding a custom "Section name" [0], does not result in the Section name being displayed as a heading on the Site homepage.

      Action: Edited summary for the front page, enabling and adding a custom "Section name" value. Saved changes.

      Expected results: Custom "Section name" should appear as a title at the top of the summary.
      Actual results: No title is displayed.

      Patch to fix this attached.

      [0] http://docs.moodle.org/22/en/course/editsection

        Gliffy Diagrams

          Activity

          Hide
          poltawski Dan Poltawski added a comment -

          Thanks for the Patch, David! I've put it into a git branch here for review. I've tested it and it does as suggested.

          But I have question of whether this is the right solution to this problem. The front page is kind of special and perhaps it shouldn't have this setting available at all. I also noticed that conditional settings on topics show up on this section also. This probably doesn't make sense.

          Show
          poltawski Dan Poltawski added a comment - Thanks for the Patch, David! I've put it into a git branch here for review. I've tested it and it does as suggested. But I have question of whether this is the right solution to this problem. The front page is kind of special and perhaps it shouldn't have this setting available at all. I also noticed that conditional settings on topics show up on this section also. This probably doesn't make sense.
          Hide
          poltawski Dan Poltawski added a comment -

          Hi Sam,

          Whilst working on this issue I just noticed that the conditional settings are appearing the front page topic section.

          My thought is that we really shouldn't be showing these settings on the front page - just wondering if you agreed?

          (ps. Sorry for adding you as a watcher on so many issues in the past few days!)

          Show
          poltawski Dan Poltawski added a comment - Hi Sam, Whilst working on this issue I just noticed that the conditional settings are appearing the front page topic section. My thought is that we really shouldn't be showing these settings on the front page - just wondering if you agreed? (ps. Sorry for adding you as a watcher on so many issues in the past few days!)
          Hide
          balchd David Balch added a comment -

          I have sites that use the front page section summary; it's a good place to introduce the site - e.g.: http://reciprocatesite.conted.ox.ac.uk/
          Taking that away would be annoying, as I'd likely have to add it back.

          Furthermore, having the section name in a separate field feels much better than including it in the text field.

          NB: The section summary is only visible when the front page setting "Include a topic" (numsections) is enabled (that confused me for a moment there).

          Show
          balchd David Balch added a comment - I have sites that use the front page section summary; it's a good place to introduce the site - e.g.: http://reciprocatesite.conted.ox.ac.uk/ Taking that away would be annoying, as I'd likely have to add it back. Furthermore, having the section name in a separate field feels much better than including it in the text field. NB: The section summary is only visible when the front page setting "Include a topic" (numsections) is enabled (that confused me for a moment there).
          Hide
          quen Sam Marshall added a comment -

          Dan - You're saying there are conditional options for section 0? I agree, there shouldn't be. Are you going to fix it or do you want me to? (If the latter, file new issue please.)

          I hadn't noticed there were ANY section options for section 0 so that would explain why I missed it.

          Show
          quen Sam Marshall added a comment - Dan - You're saying there are conditional options for section 0? I agree, there shouldn't be. Are you going to fix it or do you want me to? (If the latter, file new issue please.) I hadn't noticed there were ANY section options for section 0 so that would explain why I missed it.
          Hide
          poltawski Dan Poltawski added a comment -

          Sam: i'll take it

          David: I'm not suggesting removing the summary, just the title part of it because it doesn't make the same sense as a section on another course page (used to refer to that section). And if you wanted a section-name like title you can just use html.

          Show
          poltawski Dan Poltawski added a comment - Sam: i'll take it David: I'm not suggesting removing the summary, just the title part of it because it doesn't make the same sense as a section on another course page (used to refer to that section). And if you wanted a section-name like title you can just use html.
          Hide
          balchd David Balch added a comment -

          Dan: Ah, ok. I take it that the front page section name isn't used in the navigation at all then.
          I guess not having conditional options the front page section means amending it as a special case, so excluding the section name would fit in with that.

          There might be an argument for keeping it so that the UI is consistent with sections in courses, but obviously it's at your discretion

          Show
          balchd David Balch added a comment - Dan: Ah, ok. I take it that the front page section name isn't used in the navigation at all then. I guess not having conditional options the front page section means amending it as a special case, so excluding the section name would fit in with that. There might be an argument for keeping it so that the UI is consistent with sections in courses, but obviously it's at your discretion
          Hide
          salvetore Michael de Raadt added a comment -

          Thanks for reporting this issue.

          We have detected that this issue has been inactive for over a year. It was reported as affecting versions that are no longer supported.

          If you believe that this issue is still relevant to current versions (2.5 and beyond), please comment on the issue. Issues left inactive for a further month will be closed.

          Michael d.

          TW9vZGxlDQo=

          Show
          salvetore Michael de Raadt added a comment - Thanks for reporting this issue. We have detected that this issue has been inactive for over a year. It was reported as affecting versions that are no longer supported. If you believe that this issue is still relevant to current versions (2.5 and beyond), please comment on the issue. Issues left inactive for a further month will be closed. Michael d. TW9vZGxlDQo=
          Hide
          cfulton Charles Fulton added a comment -

          This is still broken in 2.6.1.

          Show
          cfulton Charles Fulton added a comment - This is still broken in 2.6.1.
          Hide
          salvetore Michael de Raadt added a comment -

          Dan Poltawski: Is this still an issue? Would you like to continue this or pass it to the FRONTEND team?

          Show
          salvetore Michael de Raadt added a comment - Dan Poltawski : Is this still an issue? Would you like to continue this or pass it to the FRONTEND team?
          Hide
          balchd David Balch added a comment -

          Some of my group's sites use it, so I'll be maintaining the patch in my own repo.

          I still think it's worth having the option in core - for flexibility, and consistency with course pages.

          Show
          balchd David Balch added a comment - Some of my group's sites use it, so I'll be maintaining the patch in my own repo. I still think it's worth having the option in core - for flexibility, and consistency with course pages.
          Hide
          poltawski Dan Poltawski added a comment -

          Sorry, I dropped the ball on this - David, if you can create a git branch we can submit this issue for peer review

          Show
          poltawski Dan Poltawski added a comment - Sorry, I dropped the ball on this - David, if you can create a git branch we can submit this issue for peer review
          Hide
          balchd David Balch added a comment -

          Adding Git branch based on 2.9dev.

          It's not clear whether this change will be accepted, so I won't jump through all the hoops of creating 2.8 and 2.7 branches until I hear that it would actually be committed.

          Show
          balchd David Balch added a comment - Adding Git branch based on 2.9dev. It's not clear whether this change will be accepted, so I won't jump through all the hoops of creating 2.8 and 2.7 branches until I hear that it would actually be committed.
          Hide
          cibot CiBoT added a comment -

          Fails against automated checks.

          Checked MDL-31822 using repository: https://github.com/Dave-B/moodle.git

          More information about this report

          Show
          cibot CiBoT added a comment - Fails against automated checks. Checked MDL-31822 using repository: https://github.com/Dave-B/moodle.git master (branch: MDL_31822 | CI Job ) Coding style problems found More information about this report
          Hide
          balchd David Balch added a comment - - edited

          The coding style problem from CiBot - "(#133) Line indented incorrectly; expected 8 spaces, found 12" - is due to the whole file being indented.

          Whilst I wasn't keen to change the whole file contents, I figure that I can do it in a separate commit, and then no-one else will be bugged about it.

          (Stable branches will be left without cleanup commits, matching indentation with the surrounding code.)

          Show
          balchd David Balch added a comment - - edited The coding style problem from CiBot - "(#133) Line indented incorrectly; expected 8 spaces, found 12" - is due to the whole file being indented. Whilst I wasn't keen to change the whole file contents, I figure that I can do it in a separate commit, and then no-one else will be bugged about it. (Stable branches will be left without cleanup commits, matching indentation with the surrounding code.)
          Hide
          cibot CiBoT added a comment -

          +1 code verified against automated checks.

          Checked MDL-31822 using repository: https://github.com/Dave-B/moodle.git

          More information about this report

          Show
          cibot CiBoT added a comment - +1 code verified against automated checks. Checked MDL-31822 using repository: https://github.com/Dave-B/moodle.git master (branch: MDL_31822 | CI Job ) More information about this report
          Hide
          dmonllao David Monllaó added a comment -

          Thanks for the patch David! I'm sending this to integration, I've just added an extra commit refining a few things:

          • Passing the name through format_string()
          • Changed to h3 according to how we display section names in format_section_renderer_base::section_header()

          IMO this should be only master as we are introducing changes in the UI. Adding ui_change and docs_required labels.

          I agree with you about your second commit, not sure what integrators will think about it, initially I'm setting the pull branch to your first commit + mine and leave a comment pointing to another branch including the coding style changes, I think that this approach is safer. BTW, I've amended this coding style commit as there was a PHP syntax error (missing comma) https://github.com/Dave-B/moodle/compare/MDL_31822#diff-828e0013b8f3bc1bb22b4f57172b019dR132

          Show
          dmonllao David Monllaó added a comment - Thanks for the patch David! I'm sending this to integration, I've just added an extra commit refining a few things: Passing the name through format_string() Changed to h3 according to how we display section names in format_section_renderer_base::section_header() IMO this should be only master as we are introducing changes in the UI. Adding ui_change and docs_required labels. I agree with you about your second commit, not sure what integrators will think about it, initially I'm setting the pull branch to your first commit + mine and leave a comment pointing to another branch including the coding style changes, I think that this approach is safer. BTW, I've amended this coding style commit as there was a PHP syntax error (missing comma) https://github.com/Dave-B/moodle/compare/MDL_31822#diff-828e0013b8f3bc1bb22b4f57172b019dR132
          Hide
          dmonllao David Monllaó added a comment -

          Sending this to integration, let's see. As an example (the University I used to work for) of possible section name use http://moodle.urv.cat/moodle/, I guess it would not be the only institution interested on this.

          As commented I leave here an extra commit fixing index.php coding style as submitted by David Balch (this branch is on top of the pull branch).

          git pull git://github.com/dmonllao/moodle.git MDL-31822_master-codingstyle

          Show
          dmonllao David Monllaó added a comment - Sending this to integration, let's see. As an example (the University I used to work for) of possible section name use http://moodle.urv.cat/moodle/ , I guess it would not be the only institution interested on this. As commented I leave here an extra commit fixing index.php coding style as submitted by David Balch (this branch is on top of the pull branch). git pull git://github.com/dmonllao/moodle.git MDL-31822 _master-codingstyle
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          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
          dmonllao David Monllaó added a comment -

          I've rebased both branches

          Show
          dmonllao David Monllaó added a comment - I've rebased both branches
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Accordingly with MDL-43233, fixing the whole coding-style of that file in master is ok, so np with that.

          About the fix itself, I really think it's clearly a bug, where an existing setting is incorrectly ignored, so I'd suggest to provide fixes for all supported branches.

          Finally, about the heading level... I know that normal courses have sections as h3... but in the context of frontpage I feel it's better to be h2 (like "Available courses/categories/combo" headings are). Please discuss it there, I'm not really strong about that, just found it better in frontpage.

          I'll keep this open for some hours...ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Accordingly with MDL-43233 , fixing the whole coding-style of that file in master is ok, so np with that. About the fix itself, I really think it's clearly a bug, where an existing setting is incorrectly ignored, so I'd suggest to provide fixes for all supported branches. Finally, about the heading level... I know that normal courses have sections as h3... but in the context of frontpage I feel it's better to be h2 (like "Available courses/categories/combo" headings are). Please discuss it there, I'm not really strong about that, just found it better in frontpage. I'll keep this open for some hours...ciao
          Hide
          dmonllao David Monllaó added a comment -

          Changing to h2, as there is a single section on the front page and the section name could be considered another 'section' like 'Available courses'...

          I've updated the pull branches including the coding style fixes although I've only added it to master.

          Show
          dmonllao David Monllaó added a comment - Changing to h2, as there is a single section on the front page and the section name could be considered another 'section' like 'Available courses'... I've updated the pull branches including the coding style fixes although I've only added it to master.
          Hide
          cibot CiBoT added a comment -

          Fails against automated checks.

          Checked MDL-31822 using repository: https://github.com/dmonllao/moodle.git

          More information about this report

          Show
          cibot CiBoT added a comment - Fails against automated checks. Checked MDL-31822 using repository: https://github.com/dmonllao/moodle.git MOODLE_27_STABLE (1 errors / 0 warnings) [branch: MDL-31822_27 | CI Job ] phplint (0/0) , php (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , MOODLE_28_STABLE (1 errors / 0 warnings) [branch: MDL-31822_28 | CI Job ] phplint (0/0) , php (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , master (0 errors / 0 warnings) [branch: MDL-31822_master | CI Job ] More information about this report
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Integrated (27, 28 and master), thanks!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (27, 28 and master), thanks!
          Hide
          ankit_frenz Ankit Agarwal added a comment -

          Works as described.

          Passing. Thanks!

          Show
          ankit_frenz Ankit Agarwal added a comment - Works as described. Passing. Thanks!
          Hide
          poltawski Dan Poltawski added a comment -

          Thanks for your contributions! This change is now part of Moodle in git and will be available to our standard packages at https://download.moodle.org/ shortly

          The cardinal rule of writing unmaintainable code is to specify each fact in as many places as possible and in as many ways as possible.
          – Roedy Green

          Show
          poltawski Dan Poltawski added a comment - Thanks for your contributions! This change is now part of Moodle in git and will be available to our standard packages at https://download.moodle.org/ shortly The cardinal rule of writing unmaintainable code is to specify each fact in as many places as possible and in as many ways as possible. – Roedy Green
          Hide
          tsala Helen Foster added a comment -

          I'm removing the docs_required label as this seems to be a bug fix rather than adding new functionality. Please shout if I'm wrong.

          Show
          tsala Helen Foster added a comment - I'm removing the docs_required label as this seems to be a bug fix rather than adding new functionality. Please shout if I'm wrong.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                12/Jan/15

                Agile