Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0.1
    • Fix Version/s: 2.0.4
    • Component/s: Wiki (2.x)
    • Testing Instructions:
      Hide

      To test:
      1. Adding a new file using 'Files' tab in wiki
      2. Create a page using nwiki or creole format
      3. Select the file you uploaded in step 1
      4. save the page, you will be able to see the image in wik

      Please note:
      You need to test it using Collaborative wiki and Individual Wiki with group mode on/off.

      Thanks.

      Show
      To test: 1. Adding a new file using 'Files' tab in wiki 2. Create a page using nwiki or creole format 3. Select the file you uploaded in step 1 4. save the page, you will be able to see the image in wik Please note: You need to test it using Collaborative wiki and Individual Wiki with group mode on/off. Thanks.
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Pull Master Branch:
      s9_MDL-26392_wiki_files_master
    • Rank:
      16347

      Description

      As reported by Gisela Hillenbrand in http://moodle.org/mod/forum/discuss.php?d=168577

      I use the NWiki format. Clicking on the image icon in the wiki page edit mode results in the piece of code: [[image:Image|alt]] - so where can I upload a image.jpg file and link it with that Image in my wiki?

        Issue Links

          Activity

          Hide
          Dongsheng Cai added a comment -

          Helen, we never support images for nwiki format, this is more like a new feature, not a blocker, I'm not sure if we should implement this in this sprint, I will check with Martin.

          Show
          Dongsheng Cai added a comment - Helen, we never support images for nwiki format, this is more like a new feature, not a blocker, I'm not sure if we should implement this in this sprint, I will check with Martin.
          Hide
          Martin Dougiamas added a comment -

          Can't be added in an existing sprint ...

          Show
          Martin Dougiamas added a comment - Can't be added in an existing sprint ...
          Hide
          Martin Dougiamas added a comment -

          Sorry, I was confused. It was in the sprint already

          I guess image support is a whole new feature, yes. But the fact that the bogus image button is there is a bug, so that should be fixed.

          Show
          Martin Dougiamas added a comment - Sorry, I was confused. It was in the sprint already I guess image support is a whole new feature, yes. But the fact that the bogus image button is there is a bug, so that should be fixed.
          Hide
          Dongsheng Cai added a comment -

          Martin, the image button is not completely not working, it is just not working with the new file system, if you put a image url there (I means external image file), it will be displayed.

          Show
          Dongsheng Cai added a comment - Martin, the image button is not completely not working, it is just not working with the new file system, if you put a image url there (I means external image file), it will be displayed.
          Hide
          Martin Dougiamas added a comment -

          OK, so after discussion I think it's worth it to do this in stable:

          1) Add a filemanager form element to the nwiki editing form so that images (only, for now) can be uploaded
          2) Make sure the nwiki parser can securely convert [image:somefile.jpg] into a HTML img tag via pluginfile.php

          The larger problem about other file types needs to be thought about some other time. Potential here for people to start adding huge file collections and perhaps avoid quotas etc.

          Show
          Martin Dougiamas added a comment - OK, so after discussion I think it's worth it to do this in stable: 1) Add a filemanager form element to the nwiki editing form so that images (only, for now) can be uploaded 2) Make sure the nwiki parser can securely convert [image:somefile.jpg] into a HTML img tag via pluginfile.php The larger problem about other file types needs to be thought about some other time. Potential here for people to start adding huge file collections and perhaps avoid quotas etc.
          Hide
          David Mudrak added a comment -

          I just added a link to MDL-25629 that is kind of similar issue, affecting all Moodle (not just single module).

          Show
          David Mudrak added a comment - I just added a link to MDL-25629 that is kind of similar issue, affecting all Moodle (not just single module).
          Hide
          Dongsheng Cai added a comment -
          Show
          Dongsheng Cai added a comment - Adding Aparup here for peer review: https://github.com/dongsheng/moodle/commit/119e7d4884bfc13ef234280a8e18764b9628facc Thanks!
          Hide
          Aparup Banerjee added a comment -

          code looks good, saving works fine .

          need to fix the preview pages though as images aren't showing up there (ie: draft files)

          Show
          Aparup Banerjee added a comment - code looks good, saving works fine . need to fix the preview pages though as images aren't showing up there (ie: draft files)
          Hide
          Aparup Banerjee added a comment -

          Hi DS,
          the updated fix now works, draft files and saved files all showing up nice and well! it works for me!

          Show
          Aparup Banerjee added a comment - Hi DS, the updated fix now works, draft files and saved files all showing up nice and well! it works for me!
          Hide
          Dongsheng Cai added a comment -

          PULL request submitted.

          Show
          Dongsheng Cai added a comment - PULL request submitted.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Dong, plz, take a look to my comment (when testing) at:

          http://tracker.moodle.org/browse/PULL-377?focusedCommentId=105851&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-105851

          Feel free to reply there or here... TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Dong, plz, take a look to my comment (when testing) at: http://tracker.moodle.org/browse/PULL-377?focusedCommentId=105851&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-105851 Feel free to reply there or here... TIA and ciao
          Hide
          Helen Foster added a comment -

          Linking to MDL-26782 as both issues are about images in a wiki.

          Show
          Helen Foster added a comment - Linking to MDL-26782 as both issues are about images in a wiki.
          Hide
          Ludo ( Marc Alier) added a comment -

          Hi,
          you guys did not like the way it was code the file uploading so you commented all the lines. If you provide some insight on how to do it we may add the feature again.
          L.

          Show
          Ludo ( Marc Alier) added a comment - Hi, you guys did not like the way it was code the file uploading so you commented all the lines. If you provide some insight on how to do it we may add the feature again. L.
          Hide
          Dongsheng Cai added a comment -

          Martin,

          Can you please review my changes?

          repo: git@github.com:dongsheng/moodle.git
          Branch: s8_MDL-26392_wiki_files
          diff: https://github.com/dongsheng/moodle/compare/master...s8_MDL-26392_wiki_files

          Show
          Dongsheng Cai added a comment - Martin, Can you please review my changes? repo: git@github.com:dongsheng/moodle.git Branch: s8_ MDL-26392 _wiki_files diff: https://github.com/dongsheng/moodle/compare/master...s8_MDL-26392_wiki_files
          Hide
          Dongsheng Cai added a comment -

          PULL requests submitted

          Show
          Dongsheng Cai added a comment - PULL requests submitted
          Hide
          Petr Škoda added a comment -

          Reopening, see PULL for details.

          Show
          Petr Škoda added a comment - Reopening, see PULL for details.
          Hide
          Dongsheng Cai added a comment -

          1/ require_course_login() is not enough when modifying any text or file - luckily since 2.0 guests and not-logged-in do not have any write caps, but I think we should use the require_login() x require_course_login() correctly

          2/ you can not bump version above the later branch 2011040500, it has to use single +1 increments now that we branched

          3/ each file is using different "@package mod-wiki-2.0"

          4/ why can not students manage files by default?

          5/ please use s() when adding any html element attributes - such as in: $html .= "<img src=\"$CFG->wwwroot/mod/wiki/editors/wiki/images/$button[0]\" alt=\"".$button[1]."\" title=\"".$button[1]."\" />";

          6/ do not make empty line after initial "<?php" - it helps some IDEs a lot

          7/ $groupanduser = optional_param('groupanduser', 0, PARAM_TEXT); is definitely very bad coding style

          8/ mod/wiki/filesedit.php accepts both pageid and subwiki id, why? you can derive the subwiki from the pageid, but if you have both you need to verify they match - you must not use them separately without this validation; the same problem might be in other files

          9/ mod/wiki/filesedit.php why are all parameters optional? That does not make much sense to me.

          Show
          Dongsheng Cai added a comment - 1/ require_course_login() is not enough when modifying any text or file - luckily since 2.0 guests and not-logged-in do not have any write caps, but I think we should use the require_login() x require_course_login() correctly 2/ you can not bump version above the later branch 2011040500, it has to use single +1 increments now that we branched 3/ each file is using different "@package mod-wiki-2.0" 4/ why can not students manage files by default? 5/ please use s() when adding any html element attributes - such as in: $html .= "<img src=\"$CFG->wwwroot/mod/wiki/editors/wiki/images/$button [0] \" alt=\"".$button [1] ."\" title=\"".$button [1] ."\" />"; 6/ do not make empty line after initial "<?php" - it helps some IDEs a lot 7/ $groupanduser = optional_param('groupanduser', 0, PARAM_TEXT); is definitely very bad coding style 8/ mod/wiki/filesedit.php accepts both pageid and subwiki id, why? you can derive the subwiki from the pageid, but if you have both you need to verify they match - you must not use them separately without this validation; the same problem might be in other files 9/ mod/wiki/filesedit.php why are all parameters optional? That does not make much sense to me.
          Hide
          Dongsheng Cai added a comment -

          1. replaced all require_course_login with reqire_login
          2. fixed
          3. all files use '@package mod-wiki-2.0' now
          4. Martin agreed to not allowing student manage files by default
          5. fixed by using html_writer:empty_tag
          6. What IDEs are you talking about? And what's the benefits? I removed empty lines from the files I newly created, but most moodle files have that empty line.
          7. It's actually an existing problem of wiki 2, there is a subwiki selector created in render.php, it use groupid-userid as parameter for individual wiki in group mode, it affects other places as well, I am not sure if we should fix this render function in this issue, I added clean_param for now
          8. the page id only used for navigation, it's not for file management, if I don't pass it, users won't be able to go back to the page they came from.
          9. Fixed.

          Show
          Dongsheng Cai added a comment - 1. replaced all require_course_login with reqire_login 2. fixed 3. all files use '@package mod-wiki-2.0' now 4. Martin agreed to not allowing student manage files by default 5. fixed by using html_writer:empty_tag 6. What IDEs are you talking about? And what's the benefits? I removed empty lines from the files I newly created, but most moodle files have that empty line. 7. It's actually an existing problem of wiki 2, there is a subwiki selector created in render.php, it use groupid-userid as parameter for individual wiki in group mode, it affects other places as well, I am not sure if we should fix this render function in this issue, I added clean_param for now 8. the page id only used for navigation, it's not for file management, if I don't pass it, users won't be able to go back to the page they came from. 9. Fixed.
          Hide
          Andrew Davis added a comment -

          Is it worth retaining the commented out code in mod/wiki/filesedit_form.php starting at line 45?

          Show
          Andrew Davis added a comment - Is it worth retaining the commented out code in mod/wiki/filesedit_form.php starting at line 45?
          Hide
          Dongsheng Cai added a comment -

          @Andrew

          Maybe not, I just removed it, two branches got updated.

          Show
          Dongsheng Cai added a comment - @Andrew Maybe not, I just removed it, two branches got updated.
          Hide
          Jérôme Mouneyrac added a comment -

          My peer review:
          It works. +1 for integration. I understood that some part of this code will be refactored later, so it's why I suggest to integrate it and to create issues for things to do later.

          I would suggest to write these issues:

          • some help information should explain clearly how to change the code [[image:Image|alt]]. Specially as clicking on the image button or selecting 'Insert an image...' would generate this code (a little bit buggy).
          • imo, for later, the button image should call the file picker and directly write the correct wiki code (as you would expect of anything related to file from a user point of view)
          Show
          Jérôme Mouneyrac added a comment - My peer review: It works. +1 for integration. I understood that some part of this code will be refactored later, so it's why I suggest to integrate it and to create issues for things to do later. I would suggest to write these issues: some help information should explain clearly how to change the code [ [image:Image|alt] ]. Specially as clicking on the image button or selecting 'Insert an image...' would generate this code (a little bit buggy). imo, for later, the button image should call the file picker and directly write the correct wiki code (as you would expect of anything related to file from a user point of view)
          Hide
          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
          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
          Dongsheng Cai added a comment -

          Rebased

          Regards,
          Dongsheng

          Show
          Dongsheng Cai added a comment - Rebased Regards, Dongsheng
          Hide
          Sam Hemelryk added a comment -

          Hmmmm I've just been reviewing this now and there are a couple of things I am not 100% sure about.

          1. The files link/tab is shown to everyone regardless of whether they can actually view the page.
          2. You are able to delete files that are still in use leading to broken images on pages where they were used.
          3. There is no way to access files from the HTML editor, I understand this isn't easy but it confused me while I was learning these changes.
          4. files.php - shouldn't pageid be a required param rather than optional given that the default 0 results in an exception anyway.
          5. filesedit.php displays all the tabs regardless of what the user can see/access (e.g. as a guest I am shown the edit tab).

          I also noted the following (although I wouldn't expect them to be fixed as part of this issue).

          1. The navigation is VERY confused for the wiki, especially how wiki_extend_navigation attempts to collect pageid and/or id from the URL... I think a better navigation solution could be found for the wiki module.
          2. The wiki still has inline JS calls (onchange) - these certainly need upgrading to make use of the page requirements API.
          3. There is a bug with the navigation when logged in as a guest and viewing a front page course module.
          4. The package phpdoc tag needs to be standardised and used consistently - there are still a couple of variations of the @package tag within the wiki module.

          Most of the points I have spotted are very minor or may be things that have already discussed with other reviewers, I'll wait to discuss this with Eloy before making a decision about integration.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hmmmm I've just been reviewing this now and there are a couple of things I am not 100% sure about. The files link/tab is shown to everyone regardless of whether they can actually view the page. You are able to delete files that are still in use leading to broken images on pages where they were used. There is no way to access files from the HTML editor, I understand this isn't easy but it confused me while I was learning these changes. files.php - shouldn't pageid be a required param rather than optional given that the default 0 results in an exception anyway. filesedit.php displays all the tabs regardless of what the user can see/access (e.g. as a guest I am shown the edit tab). I also noted the following (although I wouldn't expect them to be fixed as part of this issue). The navigation is VERY confused for the wiki, especially how wiki_extend_navigation attempts to collect pageid and/or id from the URL... I think a better navigation solution could be found for the wiki module. The wiki still has inline JS calls (onchange) - these certainly need upgrading to make use of the page requirements API. There is a bug with the navigation when logged in as a guest and viewing a front page course module. The package phpdoc tag needs to be standardised and used consistently - there are still a couple of variations of the @package tag within the wiki module. Most of the points I have spotted are very minor or may be things that have already discussed with other reviewers, I'll wait to discuss this with Eloy before making a decision about integration. Cheers Sam
          Hide
          Martin Dougiamas added a comment -

          A1, A4 and A5 should be fixed for sure. +1 to reject based on those. The others are probably other bugs.

          Show
          Martin Dougiamas added a comment - A1, A4 and A5 should be fixed for sure. +1 to reject based on those. The others are probably other bugs.
          Hide
          Sam Hemelryk added a comment -

          Hi DS,

          I've just been talking this issue over with Martin and Eloy, its being rejected this week so that we can clean a couple of those things up and discuss the HTML editor and file deletion points if need be.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi DS, I've just been talking this issue over with Martin and Eloy, its being rejected this week so that we can clean a couple of those things up and discuss the HTML editor and file deletion points if need be. Cheers Sam
          Hide
          Dongsheng Cai added a comment -

          Sam, Thanks for reviewing.

          A1. Fixed, user will need viewpage capability to view files
          A4. Fixed, pageid now being required
          A5. you are saying files.php right? filesedit.php doesn't display tabs, I added capability checks to tabs render.

          Regards,
          Dongsheng

          Show
          Dongsheng Cai added a comment - Sam, Thanks for reviewing. A1. Fixed, user will need viewpage capability to view files A4. Fixed, pageid now being required A5. you are saying files.php right? filesedit.php doesn't display tabs, I added capability checks to tabs render. Regards, Dongsheng
          Hide
          Rossiani Wijaya added a comment -

          Hi Dongsheng,

          I tested both patches and received the following messages:

          Notice: Undefined variable: newcontent in /m20/moodle/mod/wiki/edit.php on line 88

          Notice: Undefined variable: context in /m20/moodle/mod/wiki/pagelib.php on line 1916

          Notice: Trying to get property of non-object in /m20/moodle/mod/wiki/pagelib.php on line 1916

          To reproduce:
          1. create new wiki (either creole or nwiki)
          2. upload img file
          3. add uploaded image on edit page and save.

          also notice mod/wiki/edit.php on line 48:
          [code]
          $newconent = '';
          if (!empty($newcontent) && is_array($newcontent))

          { $newcontent = $newcontent['text']; }

          [/code]
          Variable $newconent, is this a typo?
          if it is, then what is the use of if statement for?

          Show
          Rossiani Wijaya added a comment - Hi Dongsheng, I tested both patches and received the following messages: Notice: Undefined variable: newcontent in /m20/moodle/mod/wiki/edit.php on line 88 Notice: Undefined variable: context in /m20/moodle/mod/wiki/pagelib.php on line 1916 Notice: Trying to get property of non-object in /m20/moodle/mod/wiki/pagelib.php on line 1916 To reproduce: 1. create new wiki (either creole or nwiki) 2. upload img file 3. add uploaded image on edit page and save. also notice mod/wiki/edit.php on line 48: [code] $newconent = ''; if (!empty($newcontent) && is_array($newcontent)) { $newcontent = $newcontent['text']; } [/code] Variable $newconent, is this a typo? if it is, then what is the use of if statement for?
          Hide
          Dongsheng Cai added a comment -

          Rossiani
          Those errors are not from this patch, can you please file a new bug for it?

          Show
          Dongsheng Cai added a comment - Rossiani Those errors are not from this patch, can you please file a new bug for it?
          Hide
          Rossiani Wijaya added a comment -

          File a new issue for the errors I described above: MDL-27525

          Show
          Rossiani Wijaya added a comment - File a new issue for the errors I described above: MDL-27525
          Hide
          Rossiani Wijaya added a comment -

          Hi DS,

          Apart from the errors I mentioned above, adding images in nwiki and creole works great.

          Note: to test this issue, I set 'debug messages' to none.

          Show
          Rossiani Wijaya added a comment - Hi DS, Apart from the errors I mentioned above, adding images in nwiki and creole works great. Note: to test this issue, I set 'debug messages' to none.
          Hide
          Dongsheng Cai added a comment -

          Thanks Rossiani

          I just assigned MDL-27525 myself.

          Show
          Dongsheng Cai added a comment - Thanks Rossiani I just assigned MDL-27525 myself.
          Hide
          Dongsheng Cai added a comment -

          Rossiani did you clicked "looks great to me" after peer review, this issue rolled back to development in progress

          Show
          Dongsheng Cai added a comment - Rossiani did you clicked "looks great to me" after peer review, this issue rolled back to development in progress
          Hide
          Rossiani Wijaya added a comment -

          Ds,

          I did click on "looks great to me" button after I reviewed it.

          Show
          Rossiani Wijaya added a comment - Ds, I did click on "looks great to me" button after I reviewed it.
          Hide
          Dongsheng Cai added a comment -

          Hi Rossiani

          Aparup just explained to me, either button will send issue back to in progress status, we need to click "send for integration".

          Show
          Dongsheng Cai added a comment - Hi Rossiani Aparup just explained to me, either button will send issue back to in progress status, we need to click "send for integration".
          Hide
          Rossiani Wijaya added a comment -

          Hi DS,

          This is look great. I'm sending this to integration.

          Show
          Rossiani Wijaya added a comment - Hi DS, This is look great. I'm sending this to integration.
          Hide
          Dongsheng Cai added a comment -

          rebased

          Show
          Dongsheng Cai added a comment - rebased
          Hide
          Sam Hemelryk added a comment -

          Hi Dongsheng,

          I've been reviewing this now for integration.
          All in all it looks good, I did spot several minor things but I think these could be cleaned up during integration and I've attached a patch to see them cleaned up.

          Once I've checked with Eloy that it is OK for things to be sent upstream this can go in.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Dongsheng, I've been reviewing this now for integration. All in all it looks good, I did spot several minor things but I think these could be cleaned up during integration and I've attached a patch to see them cleaned up. Once I've checked with Eloy that it is OK for things to be sent upstream this can go in. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Forgot to mention what I spotted!

          Minor things to be corrected:

          • Two new strings insertimage and insertimage_help need to be added in alphabetically.
          • wiki_print_subwiki_selector $pagetype should default to 'view' and $baseurl is assumed a moodle_url and should never default to null.
          • wiki_files_tree should be declared public as per coding style
          • wiki_parser_real_path arguments have changes but PHPdoc hasn't been updated.

          Questions I have for you DS that I needed answered before this can be integrated:

          • Is the new string cannotmanagefiles used any where/needed?
          • Is the new string insertimage_help used any where/needed?

          And general notes:

          • Test on frontpage wiki module and check access when not logged in.
          • HTML generation through $OUTPUT should be moved out of loops where possible to aid page generation time, see mod_wiki_renderer::htmllize_tree (the pix_icon calls).
          Show
          Sam Hemelryk added a comment - Forgot to mention what I spotted! Minor things to be corrected: Two new strings insertimage and insertimage_help need to be added in alphabetically. wiki_print_subwiki_selector $pagetype should default to 'view' and $baseurl is assumed a moodle_url and should never default to null. wiki_files_tree should be declared public as per coding style wiki_parser_real_path arguments have changes but PHPdoc hasn't been updated. Questions I have for you DS that I needed answered before this can be integrated: Is the new string cannotmanagefiles used any where/needed? Is the new string insertimage_help used any where/needed? And general notes: Test on frontpage wiki module and check access when not logged in. HTML generation through $OUTPUT should be moved out of loops where possible to aid page generation time, see mod_wiki_renderer::htmllize_tree (the pix_icon calls).
          Hide
          Sam Hemelryk added a comment -

          Thanks Dongsheng this has been integrated now... Hoorah

          Show
          Sam Hemelryk added a comment - Thanks Dongsheng this has been integrated now... Hoorah
          Hide
          Eloy Lafuente (stronk7) added a comment -

          passing based one Rossiani's comments.

          Show
          Eloy Lafuente (stronk7) added a comment - passing based one Rossiani's comments.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This is now upstream, yay! Many thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - This is now upstream, yay! Many thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: