Details

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

      Test prerequisites

      1. My private files with some files
      2. A file resource with some files

      Test steps

      1. Go to your privates files
      2. Click on each file and

      • make sure the button 'Set main file' is not displayed
      • using an HTML inspector (Firebug), unhide the button and make sure clicking on it does not do anything

      3. Go to the file resource

      • Make sure you can set one of the files to main file
      • Make sure you can change the main file
      • Save your changes and confirm it happened
      Show
      Test prerequisites My private files with some files A file resource with some files Test steps 1. Go to your privates files 2. Click on each file and make sure the button 'Set main file' is not displayed using an HTML inspector (Firebug), unhide the button and make sure clicking on it does not do anything 3. Go to the file resource Make sure you can set one of the files to main file Make sure you can change the main file Save your changes and confirm it happened
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-33582-master-integration

      Description

      Replication steps:-

      1. Goto to edit private files area
      2. Open file manager and edit the css to remove "display:none" attribute on the "set as main file" button
      3. Now "set as main file" button is visible which should not be. Clicking on the button actually works and sets the current file as main file which should not be happening.
      4. I was also able to see "link to file directly" option following the same method in some cases, but was not really able to get that working. In any case both situations should be investigated.

        Gliffy Diagrams

          Activity

          Hide
          salvetore Michael de Raadt added a comment -

          Yes, ideally we should not be outputting controls and then hiding them. The control should not be output at all.

          Show
          salvetore Michael de Raadt added a comment - Yes, ideally we should not be outputting controls and then hiding them. The control should not be output at all.
          Hide
          poltawski Dan Poltawski added a comment -

          The key question is whether this is checked server side.

          Show
          poltawski Dan Poltawski added a comment - The key question is whether this is checked server side.
          Hide
          poltawski Dan Poltawski added a comment -

          Assigning to Marina for peer review.

          Show
          poltawski Dan Poltawski added a comment - Assigning to Marina for peer review.
          Hide
          marina Marina Glancy added a comment -

          Hi Fred,
          The function filemanager_js_templates() is called once per page even when there are several filemanagers. So with your changes it will be called with the options of the first filemanager on the page. I suggest you to discard changes to renderer.

          So let's assume that button is always present but it is shown conditionally with CSS. And inside

          selectnode.one('.fp-file-setmain').on('click', function(e) {

          after e.preventDefault(); you may insert the condition.

          Another thing that somebody mentioned that we can disable this on server side. It will be in repository/draftfiles_ajax.php, look for "case 'setmainfile'"

          Show
          marina Marina Glancy added a comment - Hi Fred, The function filemanager_js_templates() is called once per page even when there are several filemanagers. So with your changes it will be called with the options of the first filemanager on the page. I suggest you to discard changes to renderer. So let's assume that button is always present but it is shown conditionally with CSS. And inside selectnode.one('.fp-file-setmain').on('click', function(e) { after e.preventDefault(); you may insert the condition. Another thing that somebody mentioned that we can disable this on server side. It will be in repository/draftfiles_ajax.php, look for "case 'setmainfile'"
          Hide
          fred Frédéric Massart added a comment -

          Hi Marina,

          the patch is updated, but I wonder how to correctly disable this on the server side. As I understood, mainfile is a parameter set for Filemanager on the spot, which means that draftfile_ajax.php cannot figure out itself if the files can be reordered or not. And sending the mainfile parameter via Ajax does not make much sense as if someone hacks the HTML/JS to set a main file, he would hack the parameters sent too.

          How did you think about implementing this?

          Cheers!

          Show
          fred Frédéric Massart added a comment - Hi Marina, the patch is updated, but I wonder how to correctly disable this on the server side. As I understood, mainfile is a parameter set for Filemanager on the spot, which means that draftfile_ajax.php cannot figure out itself if the files can be reordered or not. And sending the mainfile parameter via Ajax does not make much sense as if someone hacks the HTML/JS to set a main file, he would hack the parameters sent too. How did you think about implementing this? Cheers!
          Hide
          marina Marina Glancy added a comment -

          Fred, that looks good.
          Probably you are right and it would be difficult on server side. Let's forget about it

          Show
          marina Marina Glancy added a comment - Fred, that looks good. Probably you are right and it would be difficult on server side. Let's forget about it
          Hide
          poltawski Dan Poltawski added a comment -

          (note, if this applied to anything like deleting then we clearly need to ensure there is a way to do it server side)

          Show
          poltawski Dan Poltawski added a comment - (note, if this applied to anything like deleting then we clearly need to ensure there is a way to do it server side)
          Hide
          fred Frédéric Massart added a comment -

          I confirmed with Marina that the delete button is not linked to this issue. Actually, the delete button is never hidden, and even if it doesn't appear that there are checks on server side, this would be only affecting the draft, which does not change the content until the form has been submitted. I tried to open two 'My private files' within the same session, deleting a file on one side and saving on the other, the file has not been deleted.

          Pushing for integration

          Show
          fred Frédéric Massart added a comment - I confirmed with Marina that the delete button is not linked to this issue. Actually, the delete button is never hidden, and even if it doesn't appear that there are checks on server side, this would be only affecting the draft, which does not change the content until the form has been submitted. I tried to open two 'My private files' within the same session, deleting a file on one side and saving on the other, the file has not been deleted. Pushing for integration
          Hide
          poltawski Dan Poltawski added a comment -

          Thanks Fred,

          integrated

          Show
          poltawski Dan Poltawski added a comment - Thanks Fred, integrated
          Hide
          phalacee Jason Fowler added a comment -

          All good

          Show
          phalacee Jason Fowler added a comment - All good
          Hide
          stronk7 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
          stronk7 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:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                25/Jun/12