Details

    • Testing Instructions:
      Hide
      1. Create a course adding overview file to it
      2. Make sure it is displayed in courses list
      3. Edit course and change file
      4. Change settings (Appearance > Courses) to allow more files and non-image files
      5. Add more files, make sure they are displayed
      6. Login as teacher without capability moodle/course:changesummary, make sure you can't change summary and course overview files
      7. Set $CFG->courseoverviewfileslimit to 0
      8. Make sure files are no longer displayed and course edit form does not have a field for it
      Show
      Create a course adding overview file to it Make sure it is displayed in courses list Edit course and change file Change settings (Appearance > Courses) to allow more files and non-image files Add more files, make sure they are displayed Login as teacher without capability moodle/course:changesummary, make sure you can't change summary and course overview files Set $CFG->courseoverviewfileslimit to 0 Make sure files are no longer displayed and course edit form does not have a field for it
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      wip-MDL-38708-master
    • Rank:
      48758

      Description

      Add images to the course, add support for editing, backup/restore, browsing in Server files repository.

      Show the first image in the course list next to the course summary

      Please note that in MDL-38415 the "course overview files" are renamed to "course summary files" and the course edit form is changed

      1. course_edit.png
        144 kB
      2. course_settings.png
        63 kB
      3. courses list.png
        164 kB
      4. MDL-38708.jpg
        58 kB
      5. server_files.png
        24 kB

        Issue Links

          Activity

          Hide
          Martin Dougiamas added a comment -

          AWESOME!!!!!!!!!! :-D

          Show
          Martin Dougiamas added a comment - AWESOME!!!!!!!!!! :-D
          Hide
          Damyon Wiese 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.

          Thanks!

          Show
          Damyon Wiese 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. Thanks!
          Hide
          Aparup Banerjee added a comment -

          Hi Marina,
          MDL-37009 still blocking so integration can't quite proceed. (we'll put a integration_held once this gets past review). i've looked at this and here's my comments:

          • course_overviewfiles_options($course) : it may be just a coding style (i'm seeing elsewhere too) but should $course default to null in the function definition instead of expecting null as a param to be passed in?
          • the course list page might need the image (.coursebox .content .courseimage ...) more centered under the course name or perhaps just padded a little. try putting up a portrait image and you will see its off to the left too much. perhaps img should start in line with title and facilitators etc.

          that's all, nice work!

          Show
          Aparup Banerjee added a comment - Hi Marina, MDL-37009 still blocking so integration can't quite proceed. (we'll put a integration_held once this gets past review). i've looked at this and here's my comments: course_overviewfiles_options($course) : it may be just a coding style (i'm seeing elsewhere too) but should $course default to null in the function definition instead of expecting null as a param to be passed in? the course list page might need the image (.coursebox .content .courseimage ...) more centered under the course name or perhaps just padded a little. try putting up a portrait image and you will see its off to the left too much. perhaps img should start in line with title and facilitators etc. that's all, nice work!
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Marina Glancy added a comment -

          Hi Apu,
          passing null to course_overviewfiles_options() is a rare case when course is being created. I would not like to make the argument optional because that's the one and only case the argument is not required.
          Re CSS and alignment - I would much rather move teachers to the left to be aligned with course name, have no idea why it was padded so much in the first place. Can we better create an issue for Mary or whoever else takes care of core themes?

          Show
          Marina Glancy added a comment - Hi Apu, passing null to course_overviewfiles_options() is a rare case when course is being created. I would not like to make the argument optional because that's the one and only case the argument is not required. Re CSS and alignment - I would much rather move teachers to the left to be aligned with course name, have no idea why it was padded so much in the first place. Can we better create an issue for Mary or whoever else takes care of core themes?
          Hide
          Marina Glancy added a comment -

          resubmitting for integration

          Show
          Marina Glancy added a comment - resubmitting for integration
          Hide
          Aparup Banerjee added a comment -

          Thanks for creating the issue Marina. this has been integrated into master now.

          Show
          Aparup Banerjee added a comment - Thanks for creating the issue Marina. this has been integrated into master now.
          Hide
          Aparup Banerjee added a comment -

          works for me

          Show
          Aparup Banerjee added a comment - works for me
          Hide
          Aparup Banerjee added a comment -

          hmm looks like courselib_testcase::test_create_course needs to be updated

          Show
          Aparup Banerjee added a comment - hmm looks like courselib_testcase::test_create_course needs to be updated
          Hide
          Mary Evans added a comment - - edited

          @Marina: Re: CSS & alignment:
          It's not padding it's a default setting added by the browser for un-ordered list and enhanced by YUI cssbase/base.css for all ol, ul, dl

          {margin-left: 2em}
          ul.teachers { margin: 0;} will fix this problem.

          There is also a bottom margin in h3.name which can also be removed.

          Show
          Mary Evans added a comment - - edited @Marina: Re: CSS & alignment: It's not padding it's a default setting added by the browser for un-ordered list and enhanced by YUI cssbase/base.css for all ol, ul, dl {margin-left: 2em} ul.teachers { margin: 0;} will fix this problem. There is also a bottom margin in h3.name which can also be removed.
          Hide
          Mary Evans added a comment - - edited

          Why not make the summary and info containers 100% width. That way the image added in the summary will show beneath the course title. See uploaded image. https://tracker.moodle.org/secure/attachment/32107/MDL-38708.jpg

          Show
          Mary Evans added a comment - - edited Why not make the summary and info containers 100% width. That way the image added in the summary will show beneath the course title. See uploaded image. https://tracker.moodle.org/secure/attachment/32107/MDL-38708.jpg
          Hide
          Marina Glancy added a comment -

          Hi Mary,
          There are as many suggestions on CSS as there are people. I am not a designer and it takes me long-long hours to do anything decent with CSS. At the moment I have just left the look as it was before but changed class names and removed unused code.
          There is an issue MDL-38594 to fix all other themes that I did not change in MDL-37009, do you want to work on it together and make improvements to CSS for the new course listings?
          Marina

          Show
          Marina Glancy added a comment - Hi Mary, There are as many suggestions on CSS as there are people. I am not a designer and it takes me long-long hours to do anything decent with CSS. At the moment I have just left the look as it was before but changed class names and removed unused code. There is an issue MDL-38594 to fix all other themes that I did not change in MDL-37009 , do you want to work on it together and make improvements to CSS for the new course listings? Marina
          Hide
          Marina Glancy added a comment -

          Apu, I added commit changing test.

          Show
          Marina Glancy added a comment - Apu, I added commit changing test.
          Hide
          Damyon Wiese added a comment -

          I'll look at this now to try and clear the current fails.

          Show
          Damyon Wiese added a comment - I'll look at this now to try and clear the current fails.
          Hide
          Damyon Wiese added a comment -

          Thanks Marina,

          All looks good and this fixes the phpunit fail.

          Integrated the extra fix to master.

          Show
          Damyon Wiese added a comment - Thanks Marina, All looks good and this fixes the phpunit fail. Integrated the extra fix to master.
          Hide
          Mary Evans added a comment -

          @Marina,
          Sound like a good idea.
          Where do you want me to start? Here or in the ones that have been integrated...if any?

          Show
          Mary Evans added a comment - @Marina, Sound like a good idea. Where do you want me to start? Here or in the ones that have been integrated...if any?
          Hide
          Damyon Wiese added a comment -

          Hi Marina,

          This test passed - I found an issue with the maxbytes setting and this new feature - will file a new bug for that.

          Thanks, Damyon

          Show
          Damyon Wiese added a comment - Hi Marina, This test passed - I found an issue with the maxbytes setting and this new feature - will file a new bug for that. Thanks, Damyon
          Hide
          Aparup Banerjee added a comment -

          \o/

          @Mary i think working on integration would be great, otherwise you could wait for the next roll coming soon.

          Show
          Aparup Banerjee added a comment - \o/ @Mary i think working on integration would be great, otherwise you could wait for the next roll coming soon.
          Hide
          Dan Poltawski added a comment -

          Did you remember to call thankDevelopers() for 'this_weeks_work'? Defaulting to PARAM_SHODDY thanking.

          line 1289 of \lib\changes.php: call to debugging()
          line 281 of \lib\are.php: call to moodleform->detectMissingThanks()
          line 202 of \lib\now.php: call to moodleform->_is_poor_form()
          line 73 of \course\upstream.php: call to moodleform->forgetingToThank()

          Show
          Dan Poltawski added a comment - Did you remember to call thankDevelopers() for 'this_weeks_work'? Defaulting to PARAM_SHODDY thanking. line 1289 of \lib\changes.php: call to debugging() line 281 of \lib\are.php: call to moodleform->detectMissingThanks() line 202 of \lib\now.php: call to moodleform->_is_poor_form() line 73 of \course\upstream.php: call to moodleform->forgetingToThank()
          Hide
          Mary Cooch added a comment -

          Removing the qa_test_required label as there is a test for this now MDLQA-5255 ready for the next cycle.

          Show
          Mary Cooch added a comment - Removing the qa_test_required label as there is a test for this now MDLQA-5255 ready for the next cycle.
          Hide
          Marina Glancy added a comment -

          Hi Mary (lazydaisy), there is an issue MDL-38594 for changing other themes. Can we move theme-related discussion there?

          Show
          Marina Glancy added a comment - Hi Mary (lazydaisy), there is an issue MDL-38594 for changing other themes. Can we move theme-related discussion there?
          Hide
          Mary Cooch added a comment -

          Removing docs_required label as this is documented here http://docs.moodle.org/25/en/Course_settings and http://docs.moodle.org/25/en/Course_list

          Show
          Mary Cooch added a comment - Removing docs_required label as this is documented here http://docs.moodle.org/25/en/Course_settings and http://docs.moodle.org/25/en/Course_list
          Hide
          David Mudrak added a comment -

          Marina Glancy wrt the string $string['configcourseoverviewfileslimit'] may I suggest (for future development) to try and follow the suggestion documented at http://docs.moodle.org/dev/Help_strings#String_names If the string was put into moodle.php and its name was courseoverviewfiles_desc it would definitely help translators to understand the context of the string (as they would be displayed together). TIA

          Show
          David Mudrak added a comment - Marina Glancy wrt the string $string ['configcourseoverviewfileslimit'] may I suggest (for future development) to try and follow the suggestion documented at http://docs.moodle.org/dev/Help_strings#String_names If the string was put into moodle.php and its name was courseoverviewfiles_desc it would definitely help translators to understand the context of the string (as they would be displayed together). TIA
          Hide
          Marina Glancy added a comment -

          David, I named it "course overview files" and then Fred changed the string value to "course summary files" and MD agreed. Myself I find it confusing because there is a textarea "course summary" next to it. Sorry did not quite understand what were you proposing

          Show
          Marina Glancy added a comment - David, I named it "course overview files" and then Fred changed the string value to "course summary files" and MD agreed. Myself I find it confusing because there is a textarea "course summary" next to it. Sorry did not quite understand what were you proposing
          Hide
          David Mudrak added a comment -

          Sorry for not being clear. I was not referring to the string values, but to the string identifiers ($string array keys). See the linked page for examples. No offence at all, it just caught my eyes

          Show
          David Mudrak added a comment - Sorry for not being clear. I was not referring to the string values, but to the string identifiers ($string array keys). See the linked page for examples. No offence at all, it just caught my eyes
          Hide
          Marina Glancy added a comment -

          btw David, I just found issue MDL-39026 about re-phrasing

          Show
          Marina Glancy added a comment - btw David, I just found issue MDL-39026 about re-phrasing

            People

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

              Dates

              • Created:
                Updated:
                Resolved: