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
    • Rank:
      32802

      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.

      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

        Petr Škoda created issue -
        Hide
        Martin Dougiamas added a comment -

        Sam, let's chat about this.

        Show
        Martin Dougiamas added a comment - Sam, let's chat about this.
        Martin Dougiamas made changes -
        Field Original Value New Value
        Assignee moodle.com [ moodle.com ] Sam Hemelryk [ samhemelryk ]
        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
        Sam Hemelryk made changes -
        Attachment MDL-22854.20100702.01.patch [ 20608 ]
        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
        Sam Hemelryk made changes -
        Attachment MDL-22854.20100702.02.patch [ 20609 ]
        Attachment enroladd.gif [ 20610 ]
        Attachment enrolremove.gif [ 20611 ]
        Sam Hemelryk made changes -
        Attachment MDL-22854.20100702.01.patch [ 20608 ]
        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
        Sam Hemelryk made changes -
        Attachment sprite.png [ 20612 ]
        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
        Sam Hemelryk made changes -
        Attachment MDL-22854.20100702.03.patch [ 20613 ]
        Hide
        Petr Škoda 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 Škoda 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
        Sam Hemelryk made changes -
        Attachment MDL-22854.20100705.patch [ 20628 ]
        Sam Hemelryk made changes -
        Attachment MDL-22854.20100702.03.patch [ 20613 ]
        Sam Hemelryk made changes -
        Attachment MDL-22854.20100702.02.patch [ 20609 ]
        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
        Sam Hemelryk made changes -
        Attachment MDL_22854.screenshot.png [ 20631 ]
        Sam Hemelryk made changes -
        Attachment enrolremove.gif [ 20611 ]
        Hide
        Petr Škoda 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 Škoda 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.
        Martin Dougiamas made changes -
        Summary UI for not-enrolled users with role in course Add AJAX and more features to the main enrolment overview
        Priority Major [ 3 ] Blocker [ 1 ]
        Description The idea is to completely remove role_assignment interface at the course level, we already have enrolled users UI which allows users to assign roles and groups.
        The only potential problem is how to remove course administrators and other system wide people that teachers should not see.

        The current logic is in other areas is:
        1/ course participants page (/user/index.php) that is the page where both teachers and users see other course participants and their roles, we should only people with profile roles specified in $CFG->profileroles (usually tacher, editing teacher and student role); separate groups are in effect
        2/ enrolled users UI (/enrol/users.php) this is intended for editing teachers only with right to see and usually manage all course enrolments

        There is already a stub file in /enrol/otherusers.php - I suppose we should create a new capability which grants access to this information.

        UI features:
        1/ view all users in course or parent context
        2/ add and remove roles
        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.
        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
        Sam Hemelryk made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Martin Dougiamas made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        QA Assignee nobody
        Martin Dougiamas made changes -
        Workflow jira [ 36840 ] MDL Workflow [ 64608 ]
        Martin Dougiamas made changes -
        Workflow MDL Workflow [ 64608 ] MDL Full Workflow [ 93904 ]

          People

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

            Dates

            • Created:
              Updated:
              Resolved: