Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0
    • Component/s: Enrolments
    • Labels:
      None
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE

      Description

      The basic enrolment overview interface needs AJAX to be reuly useful as an interface for seeing and modifying who is in the course and what they can do.

        Gliffy Diagrams

        1. MDL-22854.20100705.patch
          137 kB
          Sam Hemelryk
        1. enroladd.gif
          0.3 kB
        2. MDL_22854.screenshot.png
          137 kB
        3. sprite.png
          1.0 kB

          Activity

          Hide
          Martin Dougiamas added a comment -

          Sam, let's chat about this.

          Show
          Martin Dougiamas added a comment - Sam, let's chat about this.
          Hide
          Sam Hemelryk added a comment -

          Hi Martin,

          This is the patch for UI for enrolling users and managing roles.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Martin, This is the patch for UI for enrolling users and managing roles. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          moodle/pix/t/enroladd.gif
          moodle/pix/t/enrolremove.gif

          Show
          Sam Hemelryk added a comment - moodle/pix/t/enroladd.gif moodle/pix/t/enrolremove.gif
          Hide
          Sam Hemelryk added a comment -

          moodle/enrol/yui/enrolmentmanager/assets/skins/sam/sprite.png

          Show
          Sam Hemelryk added a comment - moodle/enrol/yui/enrolmentmanager/assets/skins/sam/sprite.png
          Hide
          Sam Hemelryk added a comment -

          Added drag support for the enrolment panel and added a close button for the role assignment panel
          You will also need to copy the sprite.png image to
          moodle/enrol/yui/rolemanager/assets/skins/sam/sprite.png

          Show
          Sam Hemelryk added a comment - Added drag support for the enrolment panel and added a close button for the role assignment panel You will also need to copy the sprite.png image to moodle/enrol/yui/rolemanager/assets/skins/sam/sprite.png
          Hide
          Petr Skoda added a comment -

          hi!

          1/ please use define('AJAX_SCRIPT', true) - it is supposed to remove a lot of try/catch - ideally there should not be any at all
          2/ I do not like the static stuff in course_enrolment_manager
          3/ how is it supposed to work without JS, I am getting missing parameters when JS off
          4/ you must not use global $PAGE; in methods of renderable - you have to delay that till you start the actual rendering - the logic here seems to be completely reversed, the renderable should strictly work with data, the renderer methods should do the actual rendering, you have moved everything into renderable which seems wrong and it can not work when using something else than current PAGE and OUTPUT
          5/ redirect after any action - otherwise ugly things start happening when ppl press F5
          6/ you do not need to verify sesskey when data comes from mform - the test is already there
          7/ actions should not be added to $PAGE->url, always set up basic page parameters only
          8/ how do I close the overlay windows? (not finished I suppose)
          9/ how do I remove user role I just added (not finished I suppose)
          10/ maybe the course_enrolment_manager code could be in enrol/locallib.php - it will be used inside enrol/* only, right?
          11/ please use fields from the user_pictture class, I am going to tweak it a little today to accept extra fields

          Ciao

          Show
          Petr Skoda added a comment - hi! 1/ please use define('AJAX_SCRIPT', true) - it is supposed to remove a lot of try/catch - ideally there should not be any at all 2/ I do not like the static stuff in course_enrolment_manager 3/ how is it supposed to work without JS, I am getting missing parameters when JS off 4/ you must not use global $PAGE; in methods of renderable - you have to delay that till you start the actual rendering - the logic here seems to be completely reversed, the renderable should strictly work with data, the renderer methods should do the actual rendering, you have moved everything into renderable which seems wrong and it can not work when using something else than current PAGE and OUTPUT 5/ redirect after any action - otherwise ugly things start happening when ppl press F5 6/ you do not need to verify sesskey when data comes from mform - the test is already there 7/ actions should not be added to $PAGE->url, always set up basic page parameters only 8/ how do I close the overlay windows? (not finished I suppose) 9/ how do I remove user role I just added (not finished I suppose) 10/ maybe the course_enrolment_manager code could be in enrol/locallib.php - it will be used inside enrol/* only, right? 11/ please use fields from the user_pictture class, I am going to tweak it a little today to accept extra fields Ciao
          Hide
          Sam Hemelryk added a comment -

          Hi Petr

          Thank you for looking at the patch.
          I've just attached a revised patch with the following changes:

          1/ Added AJAX_SCRIPT define
          3/ Fixed the missing params when JS was turned off. It should operate exactly as it did before with JS turned off.
          4/ Totally revisited the way in which the table was being populated and rendered. The rendering is now handled prior to populating the table outside of the component.
          6/ Cleaned up sesskey confirmations for forms.
          8/ Sorry the patch missed a couple of icons (see attached images) it is functional however.
          9/ As above
          10/ Moved the course_enrolment_manager code to locallib, I think you are right will not be used outside of enrolment.
          11/ Done

          The following I have left due to the noted reasons:

          2/ The static vars were being used primarily so that there weren't available vars forcing interaction through the specified get_ methods. I haven't changed this presently however now that you know why I did it I am happy to change it if you would still like me to.
          5/ All actions when JS is turned off redirect, however currently AJAX actions for role assignment and removal do not refresh nor do I think they need to. It does refresh after new enrolments (when the overlay is closed.
          7/ The only param being set on $PAGE->url is id and filter, both of which are essential.

          Let me know your thoughts.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Petr Thank you for looking at the patch. I've just attached a revised patch with the following changes: 1/ Added AJAX_SCRIPT define 3/ Fixed the missing params when JS was turned off. It should operate exactly as it did before with JS turned off. 4/ Totally revisited the way in which the table was being populated and rendered. The rendering is now handled prior to populating the table outside of the component. 6/ Cleaned up sesskey confirmations for forms. 8/ Sorry the patch missed a couple of icons (see attached images) it is functional however. 9/ As above 10/ Moved the course_enrolment_manager code to locallib, I think you are right will not be used outside of enrolment. 11/ Done The following I have left due to the noted reasons: 2/ The static vars were being used primarily so that there weren't available vars forcing interaction through the specified get_ methods. I haven't changed this presently however now that you know why I did it I am happy to change it if you would still like me to. 5/ All actions when JS is turned off redirect, however currently AJAX actions for role assignment and removal do not refresh nor do I think they need to. It does refresh after new enrolments (when the overlay is closed. 7/ The only param being set on $PAGE->url is id and filter, both of which are essential. Let me know your thoughts. Cheers Sam
          Hide
          Petr Skoda added a comment -

          1/ enrollment with one "L" only :-D
          2/ case 'enrol': you can not enrol using any plugin, the plugin must explicitly allow it and you must check separate permission
          3/ I do not like the error handling in ajax - why not just detect the exception type? it would eliminate try/catch or if ($success) - this may need more tweaking with exception types, but in the long term it would be much much easier to write the ajax scripts imo
          4/ case 'assign': no cap test for role assign, no test if role assign even allowed in this context
          5/ why $action = optional_param('action', '', PARAM_ACTION); - it is required, right?
          6/ I think that the current use of static vars in methods breaks badly if you use multiple instances for different course sat the same time

          There seems to be some misunderstanding with define('AJAX_SCRIPT', true); - it set's up a different exception handler which encoded the exceptions using json, it also forces exceptions instead of redirects in code like require_login(), redirect(), etc. Ideally all moodle ajax should use the same structure of ajax responses encoded with json.

          going to try the ui now...

          Show
          Petr Skoda added a comment - 1/ enrollment with one "L" only :-D 2/ case 'enrol': you can not enrol using any plugin, the plugin must explicitly allow it and you must check separate permission 3/ I do not like the error handling in ajax - why not just detect the exception type? it would eliminate try/catch or if ($success) - this may need more tweaking with exception types, but in the long term it would be much much easier to write the ajax scripts imo 4/ case 'assign': no cap test for role assign, no test if role assign even allowed in this context 5/ why $action = optional_param('action', '', PARAM_ACTION); - it is required, right? 6/ I think that the current use of static vars in methods breaks badly if you use multiple instances for different course sat the same time There seems to be some misunderstanding with define('AJAX_SCRIPT', true); - it set's up a different exception handler which encoded the exceptions using json, it also forces exceptions instead of redirects in code like require_login(), redirect(), etc. Ideally all moodle ajax should use the same structure of ajax responses encoded with json. going to try the ui now...
          Hide
          Sam Hemelryk added a comment -

          Thanks for looking at that Petr,
          As per out discussion on dev chat....

          1. Close dialogues on ESC
          2. Make assign roles dialogue vertical and show desc + count so far for each
          3. Move the role assignment select out of the collapsible region within enrolment dialogue.
          4. Make a proper confirm dialogue if time permits

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks for looking at that Petr, As per out discussion on dev chat.... Close dialogues on ESC Make assign roles dialogue vertical and show desc + count so far for each Move the role assignment select out of the collapsible region within enrolment dialogue. Make a proper confirm dialogue if time permits Cheers Sam
          Hide
          Martin Dougiamas added a comment -

          I'm just renaming this to reflect what it became. See MDL-23117 for other interface.

          Show
          Martin Dougiamas added a comment - I'm just renaming this to reflect what it became. See MDL-23117 for other interface.
          Hide
          Martin Dougiamas added a comment -

          The popup should default the Assign role menu to be whatever "enrol_manual | roleid" is set in the admin settings.

          Show
          Martin Dougiamas added a comment - The popup should default the Assign role menu to be whatever "enrol_manual | roleid" is set in the admin settings.
          Hide
          Sam Hemelryk added a comment -

          Should be all done now, reopen if I've missed anything

          Show
          Sam Hemelryk added a comment - Should be all done now, reopen if I've missed anything

            People

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

              Dates

              • Created:
                Updated:
                Resolved: