Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0
    • Component/s: Backup
    • Labels:
      None
    • Difficulty:
      Difficult
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      32352

      Description

      Implement the UI for interactive backups. Must be dynamic (to accommodate new settings on the fly), respect settings dependencies / locks, really easy and solid.

        Issue Links

          Activity

          Hide
          Eloy Lafuente (stronk7) added a comment -

          $this->assertTrue(Backup UI is 100% separated from Backup process)

          Some basis about the architecture:

          • At the end, all the backup process is oriented to the execution of one backup plan.
          • Each backup plan is conformed of 1 or more backup tasks.
          • Each task can have 1 or more settings and 1 or more steps.
          • One step is the "basic" unit of execution and can be one of these:
            • execution step: executes arbitrary code.
            • exports one XML structure.
          • The behavior of the plan / tasks / steps is decided completely by the settings.

          About the settings (before UI):

          • Default values for settings can be defined in the admin interface (MDL-22140).
          • Those defaults can cause the setting to be locked for any further UI change.
          • After loading defaults, some more changes are performed depending of the Backup Mode (general backup / hub backup...)
          • Finally the security checks also can change setting values and lock them.
          • The settings follow the observer pattern and are strongly related, so some changes in some of them will, automatically cause changes in dependent settings.
          • Settings are organized in 4 levels (for now): root / course / section /activity settings.

          About the UI:

          • The UI must handle all the existing settings (and observe their built-in "dependencies" and status).
          • UI won't use session storage at all (for backup information). On each request, the whole backup_controller must be loaded from DB and, after manipulating the corresponding settings, the whole backup_controller must be saved back.
          • Is it not necessary to handle non-visible settings (unless for some reason they are needed by the UI).
          • The UI shouldn't use harcoded variables / strings / whatever but dynamically be built based in the available settings. So, if tomorrow we add one new setting... it will be automatically handled by the UI.
          • Current base_setting classes (and extensions) can be modified to fulfill all the requirements. Right now they are only "prototypes".
          Show
          Eloy Lafuente (stronk7) added a comment - $this->assertTrue(Backup UI is 100% separated from Backup process) Some basis about the architecture: At the end, all the backup process is oriented to the execution of one backup plan. Each backup plan is conformed of 1 or more backup tasks. Each task can have 1 or more settings and 1 or more steps. One step is the "basic" unit of execution and can be one of these: execution step: executes arbitrary code. exports one XML structure. The behavior of the plan / tasks / steps is decided completely by the settings. About the settings (before UI): Default values for settings can be defined in the admin interface ( MDL-22140 ). Those defaults can cause the setting to be locked for any further UI change. After loading defaults, some more changes are performed depending of the Backup Mode (general backup / hub backup...) Finally the security checks also can change setting values and lock them. The settings follow the observer pattern and are strongly related, so some changes in some of them will, automatically cause changes in dependent settings. Settings are organized in 4 levels (for now): root / course / section /activity settings. About the UI: The UI must handle all the existing settings (and observe their built-in "dependencies" and status). UI won't use session storage at all (for backup information). On each request, the whole backup_controller must be loaded from DB and, after manipulating the corresponding settings, the whole backup_controller must be saved back. Is it not necessary to handle non-visible settings (unless for some reason they are needed by the UI). The UI shouldn't use harcoded variables / strings / whatever but dynamically be built based in the available settings. So, if tomorrow we add one new setting... it will be automatically handled by the UI. Current base_setting classes (and extensions) can be modified to fulfill all the requirements. Right now they are only "prototypes".
          Hide
          Eloy Lafuente (stronk7) added a comment -

          More comments about the backup UI:

          • It should guide the user along a number of pages is order to allow him to determine all the available settings.
          • It seems logical to have t least these pages:
            1. Main page: where it's explained what is going to be backup and the root settings are displayed (but the filename that we'll leave to end on purpose).
            2. Course / section / activities page: Where one "draft" of the course / section / activity is showed, each one with its own settings. JS can be used to fulfill dependencies in the same page, but, in any case, server-side checks must be performed always. In this page, at the top, we'll include the backup filename (because with the information available in the 1st page we can already calculate the filename but not before this - see MDL-22145).
          • Any problem with the settings will cause the page to be re-displayed with information about the problem / conflict.
          • As part of the UI form validation, all the security checks will be re-executed, and one exception will be thrown if settings are violating any permission. It's UI responsibility to intercept this exception and show proper information.
          Show
          Eloy Lafuente (stronk7) added a comment - More comments about the backup UI: It should guide the user along a number of pages is order to allow him to determine all the available settings. It seems logical to have t least these pages: Main page: where it's explained what is going to be backup and the root settings are displayed (but the filename that we'll leave to end on purpose). Course / section / activities page: Where one "draft" of the course / section / activity is showed, each one with its own settings. JS can be used to fulfill dependencies in the same page, but, in any case, server-side checks must be performed always. In this page, at the top, we'll include the backup filename (because with the information available in the 1st page we can already calculate the filename but not before this - see MDL-22145 ). Any problem with the settings will cause the page to be re-displayed with information about the problem / conflict. As part of the UI form validation, all the security checks will be re-executed, and one exception will be thrown if settings are violating any permission. It's UI responsibility to intercept this exception and show proper information.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Location of backup elements:

          • For activity backups it sounds natural to have one option in the activity administration (settings block) for each activity. Alternatively, in the course page, with edition enabled, perhaps we could also show one "backup" option on each activity. Not sure, FYD. And not sure if we must show one option for each backup format available or no (right now we only have the "moodle2" format but could be adding new formats in the future).
          • For section backups, I really don't know where we can add that interface element. Perhaps in the course main page, near the expand / collapse icons of the sections. Not really sure.
          • For course backups, well, where they are now (course administration).

          Notes:

          1. In order to know if one activity supports backup, it must FEATURE_BACKUP_XXXX (where XXXX is the format of the backup).
          2. In order to know, for a given format, which "parts" can be backup (1activity / 1 section / 1 course), the corresponding plan_builder for that format has one supported_backup_types() method providing that info. So if one format doesn't support section backups its interface won't be showed based in that function results (moodle2 format supports the 3 types, btw)
          Show
          Eloy Lafuente (stronk7) added a comment - Location of backup elements: For activity backups it sounds natural to have one option in the activity administration (settings block) for each activity. Alternatively, in the course page, with edition enabled, perhaps we could also show one "backup" option on each activity. Not sure, FYD. And not sure if we must show one option for each backup format available or no (right now we only have the "moodle2" format but could be adding new formats in the future). For section backups, I really don't know where we can add that interface element. Perhaps in the course main page, near the expand / collapse icons of the sections. Not really sure. For course backups, well, where they are now (course administration). Notes: In order to know if one activity supports backup, it must FEATURE_BACKUP_XXXX (where XXXX is the format of the backup). In order to know, for a given format, which "parts" can be backup (1activity / 1 section / 1 course), the corresponding plan_builder for that format has one supported_backup_types() method providing that info. So if one format doesn't support section backups its interface won't be showed based in that function results (moodle2 format supports the 3 types, btw)
          Hide
          Martin Dougiamas added a comment -

          Thanks Eloy.

          Appreciate you are describing it in a functional way to avoid influencing the actual design too much but could you add some examples for common scenarios as you imagine them?

          eg to back up a course, first screen is X where you can set XX and XXX, second screen is Y where you can set YY, then depending on YY you see Z or ZZ or ZZZ ...

          Show
          Martin Dougiamas added a comment - Thanks Eloy. Appreciate you are describing it in a functional way to avoid influencing the actual design too much but could you add some examples for common scenarios as you imagine them? eg to back up a course, first screen is X where you can set XX and XXX, second screen is Y where you can set YY, then depending on YY you see Z or ZZ or ZZZ ...
          Hide
          Sam Hemelryk added a comment -

          Hi Eloy,

          Phew! have just attached the patch for the backup UI, a couple of things about ti first up:

          • Move styles from backup/util/ui/styles.css into the themes.
          • Get rid of backup/util/ui/includes.php and move includes into proper backup includes file.
          • Add boilerplates and actually document code (currently just PHPdoc)
          • Any changes you request

          I've tried to take in everything I can from backup but undoubtedly I have missed things (probably many) so don't hesitate to let rip and line up all the changes you desire. I should warn you about the naming, I didn't stop to put alot of thought into naming things and have decided to leave that up to you! let me know what you would like classes/functions/files renamed to and I'll be more than happy to do it.

          I'll be in Saturday and Sunday to get this done

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Eloy, Phew! have just attached the patch for the backup UI, a couple of things about ti first up: Move styles from backup/util/ui/styles.css into the themes. Get rid of backup/util/ui/includes.php and move includes into proper backup includes file. Add boilerplates and actually document code (currently just PHPdoc) Any changes you request I've tried to take in everything I can from backup but undoubtedly I have missed things (probably many) so don't hesitate to let rip and line up all the changes you desire. I should warn you about the naming, I didn't stop to put alot of thought into naming things and have decided to leave that up to you! let me know what you would like classes/functions/files renamed to and I'll be more than happy to do it. I'll be in Saturday and Sunday to get this done Cheers Sam
          Hide
          Olli Savolainen added a comment -

          Adding link to mockup based on last summer's evaluation of the 1.9 interactive backup UI that seems to have faded into oblivion.

          Of course, I have not been able to follow the development process since, so this is based on info provided by Eloy at that point.

          Show
          Olli Savolainen added a comment - Adding link to mockup based on last summer's evaluation of the 1.9 interactive backup UI that seems to have faded into oblivion. Of course, I have not been able to follow the development process since, so this is based on info provided by Eloy at that point.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Code looks really great (1st overview). Great job, Sam!

          Just applied the patch here and, after 1st stage (general settings), no matter if I press "proceed" or "cancel" I get one:

          loading controller from db
          QuickForm Error: element already exists Element 'setting_root_users' already exists in HTML_QuickForm::addElement()

          with one backtrace like kilometers long (in fact, I continued, because the schema is printed below the long backtrace and, when "proceed" again (to go to the "confirm" stage) the browser was receiving information for minutes (hyper-looong backtrace) and finished not responding, so I had to kill it (same "element already exists" problem).

          Let's name to this problem: P1 - odd backtrace problem

          No matter of that I continue examining code / behavior.... ttyl!

          Show
          Eloy Lafuente (stronk7) added a comment - Code looks really great (1st overview). Great job, Sam! Just applied the patch here and, after 1st stage (general settings), no matter if I press "proceed" or "cancel" I get one: loading controller from db QuickForm Error: element already exists Element 'setting_root_users' already exists in HTML_QuickForm::addElement() with one backtrace like kilometers long (in fact, I continued, because the schema is printed below the long backtrace and, when "proceed" again (to go to the "confirm" stage) the browser was receiving information for minutes (hyper-looong backtrace) and finished not responding, so I had to kill it (same "element already exists" problem). Let's name to this problem: P1 - odd backtrace problem No matter of that I continue examining code / behavior.... ttyl!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Olli,

          I think the UI that Sam is implementing has various of those usability issues into consideration, though I must confess I forgot completely about that linked (MDL-20036) issue.

          I think now we are really more 1-2-3-4 stage based (so it wont be complex to add the "progress information"). And at the same time course hierarchy is far better represented (we can "stylize" them latter if necessary,

          The only point I don't know if we can implement as suggested is the "Previous" functionality. As far as settings along the pages are highly dependent... perhaps it can introduce some problems right now.

          About feedback (or how the display/feedback of backup / restore processes is shown), I'd recommend you to follow / suggest at MDL-22144 where the thing will be implemented soon.

          Thanks for the link and collaboration, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Olli, I think the UI that Sam is implementing has various of those usability issues into consideration, though I must confess I forgot completely about that linked ( MDL-20036 ) issue. I think now we are really more 1-2-3-4 stage based (so it wont be complex to add the "progress information"). And at the same time course hierarchy is far better represented (we can "stylize" them latter if necessary, The only point I don't know if we can implement as suggested is the "Previous" functionality. As far as settings along the pages are highly dependent... perhaps it can introduce some problems right now. About feedback (or how the display/feedback of backup / restore processes is shown), I'd recommend you to follow / suggest at MDL-22144 where the thing will be implemented soon. Thanks for the link and collaboration, ciao
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Hi again Sam, finally I got it in and usable way (by disabling moodle's debugging completely). Cool. I've been able to complete the process!

          Anyway P1 happens, so it continues pending of investigation.

          Some more comments, numbered for reference:

          P2 - I've seen you've "hacked" a bit the activity and section tasks in order to get the activity / section labels. I think it's not really necessary as far as:

          • for backup_activity_task: The name of the task is already the name of the activity.
          • for backup_section_task: Right now the name of the task is the "number" of the section. But applying this simple patch to the factory, it will be the name of the section if available (leaving the task free from any $DB use)
          <return new backup_section_task($section->section, $sectionid);
          ---
          >return new backup_section_task(empty($section->name) ? $section->section : $section->name, $sectionid);
          

          P3 - Add some missing dependencies (my fault). I will add them once your code lands to CVS. To have them under control:

          • all the activities_included depend of the root activities one.
          • all the activities_included and activities_userinfo depend of section_included
          • all the section_userinfo depend of section_included
          • every activity_userinfo depends of activity_included
          • grade_histories depends of users

          Also, I've added, on purpose some new settings (section, activity) and played with different dependencies, just to check that the ui "adopts" them properly and results have been 100% success. Great!

          P4 - Checkboxes vs dropdowns. I remember that, originally, the backup interface used to have dropdowns for all the activities setting, but there were a global request about to change them to checkboxes (simple click vs click and drag). I think they are better in the "schema" page. No problem in the "root" page.

          P5 - Have seen "add_dependancies()" - typo?

          P6 - We have to see how the "execution" of the backup is going to work with the controller->output() singleton, that is the one in charge of "painting" all the backup progress. Just annotating it to avoid forgetting.

          I stop looking now. Want to commit some stuff before tomorrow. If I've time I'll look to code with more detail later... anyway it's looking really, really cool.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Hi again Sam, finally I got it in and usable way (by disabling moodle's debugging completely). Cool. I've been able to complete the process! Anyway P1 happens, so it continues pending of investigation. Some more comments, numbered for reference: P2 - I've seen you've "hacked" a bit the activity and section tasks in order to get the activity / section labels. I think it's not really necessary as far as: for backup_activity_task: The name of the task is already the name of the activity. for backup_section_task: Right now the name of the task is the "number" of the section. But applying this simple patch to the factory, it will be the name of the section if available (leaving the task free from any $DB use) < return new backup_section_task($section->section, $sectionid); --- > return new backup_section_task(empty($section->name) ? $section->section : $section->name, $sectionid); P3 - Add some missing dependencies (my fault). I will add them once your code lands to CVS. To have them under control: all the activities_included depend of the root activities one. all the activities_included and activities_userinfo depend of section_included all the section_userinfo depend of section_included every activity_userinfo depends of activity_included grade_histories depends of users Also, I've added, on purpose some new settings (section, activity) and played with different dependencies, just to check that the ui "adopts" them properly and results have been 100% success. Great! P4 - Checkboxes vs dropdowns. I remember that, originally, the backup interface used to have dropdowns for all the activities setting, but there were a global request about to change them to checkboxes (simple click vs click and drag). I think they are better in the "schema" page. No problem in the "root" page. P5 - Have seen "add_dependancies()" - typo? P6 - We have to see how the "execution" of the backup is going to work with the controller->output() singleton, that is the one in charge of "painting" all the backup progress. Just annotating it to avoid forgetting. I stop looking now. Want to commit some stuff before tomorrow. If I've time I'll look to code with more detail later... anyway it's looking really, really cool.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          P7 - There is one remaining print_object() - my fault - in backup/util/setting/base_setting.class.php. Feel free to delete it from your code.

          Show
          Eloy Lafuente (stronk7) added a comment - P7 - There is one remaining print_object() - my fault - in backup/util/setting/base_setting.class.php. Feel free to delete it from your code.
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          Thanks for the reviewing Eloy, I've just attached the next patch now.
          The following changes have now been made:

          • p2: Sections and activities *_include settings now get the UI label from the task name. backup_section/activity_task still set the label for the userinfo because it is a static string.
          • p4: Checkboxs are now default UI for settings, moodle2 backup_root_task settings set UI to select boxes.
          • p5: Fixed spelling mistake add_dependancies => add_dependencies.
          • p7: Removed print_object call in backup/util/settings/base.setting.class.php.
          • Cancel click is now handled properly across all stages
          • Moved CSS into base and standard theme and got rid of backup/util/ui/styles.css
          • Moved includes to backup/util/includes/backup_includes.php and removed backup/util/ui/includes.php
          • Implemented a renderer solution backup/util/ui/renderer.php
            • Added a staging progress bar through the renderer
            • Moved notification of dependency enforcement to the renderer
          • Finished php docs
          • Added boilerplates
          • Added inline documentation

          In regards to p1 I've tried to replicate it but havn't been successful yet, perhaps there was a problem in the patch that I attached? could you please see if it is still in affect after applying this patch.
          Also if you have any ideas that may help me in replicating it that would be most appreciated.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, Thanks for the reviewing Eloy, I've just attached the next patch now. The following changes have now been made: p2: Sections and activities *_include settings now get the UI label from the task name. backup_section/activity_task still set the label for the userinfo because it is a static string. p4: Checkboxs are now default UI for settings, moodle2 backup_root_task settings set UI to select boxes. p5: Fixed spelling mistake add_dependancies => add_dependencies. p7: Removed print_object call in backup/util/settings/base.setting.class.php. Cancel click is now handled properly across all stages Moved CSS into base and standard theme and got rid of backup/util/ui/styles.css Moved includes to backup/util/includes/backup_includes.php and removed backup/util/ui/includes.php Implemented a renderer solution backup/util/ui/renderer.php Added a staging progress bar through the renderer Moved notification of dependency enforcement to the renderer Finished php docs Added boilerplates Added inline documentation In regards to p1 I've tried to replicate it but havn't been successful yet, perhaps there was a problem in the patch that I attached? could you please see if it is still in affect after applying this patch. Also if you have any ideas that may help me in replicating it that would be most appreciated. Cheers Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Oki, so current status is:

          Pending:

          P1: under investigation. The huge backtrace with $CFG->debug = 38911

          P3 - Add some missing dependencies (my fault). I will add them once your code lands to CVS. To have them under control:

          • all the activities_included depend of the root activities one.
          • all the activities_included and activities_userinfo depend of section_included
          • all the section_userinfo depend of section_included
          • every activity_userinfo depends of activity_included
          • grade_histories depends of users

          Going to apply ant test patch2...

          Show
          Eloy Lafuente (stronk7) added a comment - Oki, so current status is: Pending: P1: under investigation. The huge backtrace with $CFG->debug = 38911 P3 - Add some missing dependencies (my fault). I will add them once your code lands to CVS. To have them under control: all the activities_included depend of the root activities one. all the activities_included and activities_userinfo depend of section_included all the section_userinfo depend of section_included every activity_userinfo depends of activity_included grade_histories depends of users Going to apply ant test patch2...
          Hide
          Sam Hemelryk added a comment -

          Woohooo fixed P1

          Show
          Sam Hemelryk added a comment - Woohooo fixed P1
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          hohoho, works great! So only these are pending:

          P3 - Add some missing dependencies (my fault). I will add them once your code lands to CVS. To have them under control:

          • all the activities_included depend of the root activities one.
          • all the activities_included and activities_userinfo depend of section_included
          • all the section_userinfo depend of section_included
          • every activity_userinfo depends of activity_included
          • grade_histories depends of users

          And these are the new one introduced and commented by chat.

          P6 - Integration of backup output singleton into process page ui. Anything to do?
          P8 - Lock text => locked icon
          P9 - Fixes for backup.php to properly check course/section or course/activity concordancy
          P10 - Backup.php to check user capabilities based in the type of backup (eloy)
          P11 - Possibility to add help buttons to settings (future func. not needed right now)
          P12 - Possibility to define new dependency types (future uses, not now)
          P13 - The filename setting, how should we handling it, finally.
          P14 - Commit/commit/commit!
          P15 - As far as backup.php is going to handle course/section/activity backups... something in the frontend should show /remember us which is being processed (one heading or so?)

          Show
          Eloy Lafuente (stronk7) added a comment - - edited hohoho, works great! So only these are pending: P3 - Add some missing dependencies (my fault). I will add them once your code lands to CVS. To have them under control: all the activities_included depend of the root activities one. all the activities_included and activities_userinfo depend of section_included all the section_userinfo depend of section_included every activity_userinfo depends of activity_included grade_histories depends of users And these are the new one introduced and commented by chat. P6 - Integration of backup output singleton into process page ui. Anything to do? P8 - Lock text => locked icon P9 - Fixes for backup.php to properly check course/section or course/activity concordancy P10 - Backup.php to check user capabilities based in the type of backup (eloy) P11 - Possibility to add help buttons to settings (future func. not needed right now) P12 - Possibility to define new dependency types (future uses, not now) P13 - The filename setting, how should we handling it, finally. P14 - Commit/commit/commit! P15 - As far as backup.php is going to handle course/section/activity backups... something in the frontend should show /remember us which is being processed (one heading or so?)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          P16 - Just detected that configuring one setting to be hidden backup_setting::HIDDEN (when instantiated), hides it in the form, perfect, but not in the next stage, when the summary of previous stage is showed.

          Show
          Eloy Lafuente (stronk7) added a comment - P16 - Just detected that configuring one setting to be hidden backup_setting::HIDDEN (when instantiated), hides it in the form, perfect, but not in the next stage, when the summary of previous stage is showed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          P17 - while looking in the ui stuff if you are effectively calling backup_check::check_security() at any point (directly or via call to controller->process_ui_event()). I've found it's never called! And should be, just to re-check that new configured settings don't violate any security point (caps evaluation). That function will throw exception if something fails. it's UI responsibility to catch it as necessary.

          P18 - I thing we can take rid of controller->show_ui() and controller->process_ui() as far that they aren't used with current impl, correct? controller->process_ui_event() could stay there (as used by P17), or rename it to something meaning "postprocess_hook_after_ui" or so.

          Show
          Eloy Lafuente (stronk7) added a comment - P17 - while looking in the ui stuff if you are effectively calling backup_check::check_security() at any point (directly or via call to controller->process_ui_event()). I've found it's never called! And should be, just to re-check that new configured settings don't violate any security point (caps evaluation). That function will throw exception if something fails. it's UI responsibility to catch it as necessary. P18 - I thing we can take rid of controller->show_ui() and controller->process_ui() as far that they aren't used with current impl, correct? controller->process_ui_event() could stay there (as used by P17), or rename it to something meaning "postprocess_hook_after_ui" or so.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          P19 - Not critical, but this:

          $backupid = optional_param('backup', false, PARAM_ALPHANUM);
          

          in the backup.ui.class.php. Why is it being got (obtained) there and isn't a param in the function instead?

          Show
          Eloy Lafuente (stronk7) added a comment - - edited P19 - Not critical, but this: $backupid = optional_param('backup', false , PARAM_ALPHANUM); in the backup.ui.class.php. Why is it being got (obtained) there and isn't a param in the function instead?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          P20 - One question, right now, with your changes... the old process_change() methods is the settings aren't anymore necessary, correct? dependencies have taken the control. If so... we should clean that from the settings API completely, correct?

          P21 - Imagine this case: 3 settings A, B, C:

          • A (setting_dependency_disabledif_equals = false) to C
          • B (setting_dependency_disabledif_equals = false) to C

          If I set A = false, expected result is to end with C = disabled (locked by hierarchy) and false. Current result is that, at the end, it's unlocked with value = true (because B dependency uses to undo A dependency).

          I only have been able to workaround this by also creating this dependency:

          • A (setting_dependency_disabledif_equals = false) to B

          But it could be NOT applicable in all cases. Anyone locking should cause the setting to remain locked.

          Show
          Eloy Lafuente (stronk7) added a comment - P20 - One question, right now, with your changes... the old process_change() methods is the settings aren't anymore necessary, correct? dependencies have taken the control. If so... we should clean that from the settings API completely, correct? P21 - Imagine this case: 3 settings A, B, C: A (setting_dependency_disabledif_equals = false) to C B (setting_dependency_disabledif_equals = false) to C If I set A = false, expected result is to end with C = disabled (locked by hierarchy) and false. Current result is that, at the end, it's unlocked with value = true (because B dependency uses to undo A dependency). I only have been able to workaround this by also creating this dependency: A (setting_dependency_disabledif_equals = false) to B But it could be NOT applicable in all cases. Anyone locking should cause the setting to remain locked.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          P22 - More on dependencies. I've tried to set one dependency so, if anonymize is enabled, user_files will be disabled (and value = false). And something wrong seems to be happening with the DISABLED_CHECKED/TRUE and DISABLED_NOT_CHECKED/FALSE dependencies.

          I've assumed this was of type: DISABLED_TRUE so just added:

          $anonymize->add_dependency($userfiles, setting_dependency::DISABLED_TRUE);

          but, apart from some apparent disordering in the params list and missing calls I've changed here, I cannot find why it doesn't work as expected.

          Here there are the changes I've done (not committed). Continues not working (sure it's something simple, by I got lost with values / default values and so on, sorry):

          Edited: deleted as not-useful. Already fixed by Sam below.
          

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited P22 - More on dependencies. I've tried to set one dependency so, if anonymize is enabled, user_files will be disabled (and value = false). And something wrong seems to be happening with the DISABLED_CHECKED/TRUE and DISABLED_NOT_CHECKED/FALSE dependencies. I've assumed this was of type: DISABLED_TRUE so just added: $anonymize->add_dependency($userfiles, setting_dependency::DISABLED_TRUE); but, apart from some apparent disordering in the params list and missing calls I've changed here, I cannot find why it doesn't work as expected. Here there are the changes I've done (not committed). Continues not working (sure it's something simple, by I got lost with values / default values and so on, sorry): Edited: deleted as not-useful. Already fixed by Sam below. Ciao
          Hide
          Sam Hemelryk added a comment -

          Hi Eloy,

          Thanks for getting into the UI the following changes have now been made:

          • P8 - Lock text => locked icon
          • P9 - Fixes for backup.php to properly check course/section or course/activity concordancy
          • P10 - Backup.php to check user capabilities based in the type of backup (eloy)
          • P11 - Possibility to add help buttons to settings (future func. not needed right now)
          • P12 - Possibility to define new dependency types (future uses, not now)
          • P14 - Commit/commit/commit!
          • P15 - As far as backup.php is going to handle course/section/activity backups... something in the frontend should show /remember us which is being processed (one heading or so?)
          • P16 - Just detected that configuring one setting to be hidden backup_setting::HIDDEN (when instantiated), hides it in the form, perfect, but not in the next stage, when the summary of previous stage is showed.
          • P17 - while looking in the ui stuff if you are effectively calling backup_check::check_security() at any point (directly or via call to controller->process_ui_event()). I've found it's never called! And should be, just to re-check that new configured settings don't violate any security point (caps evaluation). That function will throw exception if something fails. it's UI responsibility to catch it as necessary.
          • P18 - I thing we can take rid of controller->show_ui() and controller->process_ui() as far that they aren't used with current impl, correct? controller->process_ui_event() could stay there (as used by P17), or rename it to something meaning "postprocess_hook_after_ui" or so.
          • P19 - Not critical, but this: $backupid = optional_param('backup', false, PARAM_ALPHANUM); in the backup.ui.class.php. Why is it being got (obtained) there and isn't a param in the function instead?
          • P20 - One question, right now, with your changes... the old process_change() methods is the settings aren't anymore necessary, correct? dependencies have taken the control. If so... we should clean that from the settings API completely, correct?

          Not done yet:

          • P6 - Integration of backup output singleton into process page ui. Anything to do?
            Not too sure what you mean by this sorry? Is it the output_controller used for logging and such? If so then we could modify html_list_progress_trace to store a list of traces rather than output immediately and the write a renderer method to display those trace.
          • P13 - The filename setting, how should we handling it, finally.
            I see you added backup_plan_dbops::get_default_backup_filename however should I use it for this purpose, and if so what are the args, what is the user allowed to change?
          • P21 - This problem arises specifically because the dependent setting has no knowledge of their own dependencies nor method allowing them to check the settings they are dependent on. I'll look at creating a patch for this now.
          • P22 - Will also look at this at the same time as P21

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Eloy, Thanks for getting into the UI the following changes have now been made: P8 - Lock text => locked icon P9 - Fixes for backup.php to properly check course/section or course/activity concordancy P10 - Backup.php to check user capabilities based in the type of backup (eloy) P11 - Possibility to add help buttons to settings (future func. not needed right now) P12 - Possibility to define new dependency types (future uses, not now) P14 - Commit/commit/commit! P15 - As far as backup.php is going to handle course/section/activity backups... something in the frontend should show /remember us which is being processed (one heading or so?) P16 - Just detected that configuring one setting to be hidden backup_setting::HIDDEN (when instantiated), hides it in the form, perfect, but not in the next stage, when the summary of previous stage is showed. P17 - while looking in the ui stuff if you are effectively calling backup_check::check_security() at any point (directly or via call to controller->process_ui_event()). I've found it's never called! And should be, just to re-check that new configured settings don't violate any security point (caps evaluation). That function will throw exception if something fails. it's UI responsibility to catch it as necessary. P18 - I thing we can take rid of controller->show_ui() and controller->process_ui() as far that they aren't used with current impl, correct? controller->process_ui_event() could stay there (as used by P17), or rename it to something meaning "postprocess_hook_after_ui" or so. P19 - Not critical, but this: $backupid = optional_param('backup', false, PARAM_ALPHANUM); in the backup.ui.class.php. Why is it being got (obtained) there and isn't a param in the function instead? P20 - One question, right now, with your changes... the old process_change() methods is the settings aren't anymore necessary, correct? dependencies have taken the control. If so... we should clean that from the settings API completely, correct? Not done yet: P6 - Integration of backup output singleton into process page ui. Anything to do? Not too sure what you mean by this sorry? Is it the output_controller used for logging and such? If so then we could modify html_list_progress_trace to store a list of traces rather than output immediately and the write a renderer method to display those trace. P13 - The filename setting, how should we handling it, finally. I see you added backup_plan_dbops::get_default_backup_filename however should I use it for this purpose, and if so what are the args, what is the user allowed to change? P21 - This problem arises specifically because the dependent setting has no knowledge of their own dependencies nor method allowing them to check the settings they are dependent on. I'll look at creating a patch for this now. P22 - Will also look at this at the same time as P21 Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          P21 is not sorted, setting are now aware of both what they are dependent on and what depends on them.

          I''ve worked out what is causing P22 and am now working towards a solution.

          Show
          Sam Hemelryk added a comment - P21 is not sorted, setting are now aware of both what they are dependent on and what depends on them. I''ve worked out what is causing P22 and am now working towards a solution.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Some comments:

          • P6 - That singleton was added there as a way to separate any output from process completely. And also, allow easy extension along the time (providing other visualizations of the progress, right now it's a simple "lister"). Its only relation with loggers is that we can "flag" logger actions to be also sent to output if desired but it isn't in charge of outputting logs (each logger has its own output defined, independent of the singleton).
            So, along the next days... I'll be adding log() - backup_helper::log() - calls here and there, and some of them will have the $display param enabled, hence will be sent to singleton, also, there will be some direct calls to output_controller::get_instance->output() sending other bits to the singleton.
            So, from the backup ui POV, one "list" (html/text) will be set to output (execution stage). Just that. I don't think we need any integration (ui and output don't need to "meet" the other at all). But, perhaps, instead of the current auto-redirect at the end of the stage... we should show one "continue" button, to give the user the possibility of reading the output and so on. Just that.
          • P13: both format and type are controller's format and type. Same for id. In the other side, users and anonymised are the values of those settings. I was thinking about to set the default for filename = ''. Then, when it's firstly "rendered" or harcoded in that stage page, fill it if empty with results from the function. Also note that that filed must be validated to be non empty, PARAM_FILENAME and end with .zip (we are missing those validations right now, my fault I didn't specified them anywhere).

          Finally one comment that, while not important for 1-backup can have high relevance when implementing multiple backups (in the same request) like scheduled backups / category backups...

          I've noticed that, after putting all the ui machinery to work, in my test server, memory usage has grown from 10M to 15M for the same course. Worst, executing in the same script 5 backups, caused each one to raise memory 1M, and now it's near 2M (I guess this is because of the new circular references introduced that php isn't able to destroy properly and now we far more than before).

          So this is one problem to take into account when implementing multiple-backups unless we find one way to properly destroy all those references, so php will be able to gc that memory. Nor critical for now... but could become...

          Show
          Eloy Lafuente (stronk7) added a comment - Some comments: P6 - That singleton was added there as a way to separate any output from process completely. And also, allow easy extension along the time (providing other visualizations of the progress, right now it's a simple "lister"). Its only relation with loggers is that we can "flag" logger actions to be also sent to output if desired but it isn't in charge of outputting logs (each logger has its own output defined, independent of the singleton). So, along the next days... I'll be adding log() - backup_helper::log() - calls here and there, and some of them will have the $display param enabled, hence will be sent to singleton, also, there will be some direct calls to output_controller::get_instance->output() sending other bits to the singleton. So, from the backup ui POV, one "list" (html/text) will be set to output (execution stage). Just that. I don't think we need any integration (ui and output don't need to "meet" the other at all). But, perhaps, instead of the current auto-redirect at the end of the stage... we should show one "continue" button, to give the user the possibility of reading the output and so on. Just that. P13 : both format and type are controller's format and type. Same for id. In the other side, users and anonymised are the values of those settings. I was thinking about to set the default for filename = ''. Then, when it's firstly "rendered" or harcoded in that stage page, fill it if empty with results from the function. Also note that that filed must be validated to be non empty, PARAM_FILENAME and end with .zip (we are missing those validations right now, my fault I didn't specified them anywhere). Finally one comment that, while not important for 1-backup can have high relevance when implementing multiple backups (in the same request) like scheduled backups / category backups... I've noticed that, after putting all the ui machinery to work, in my test server, memory usage has grown from 10M to 15M for the same course. Worst, executing in the same script 5 backups, caused each one to raise memory 1M, and now it's near 2M (I guess this is because of the new circular references introduced that php isn't able to destroy properly and now we far more than before). So this is one problem to take into account when implementing multiple-backups unless we find one way to properly destroy all those references, so php will be able to gc that memory. Nor critical for now... but could become...
          Hide
          Sam Hemelryk added a comment -

          Woohoo I've just commit a solution to P22. Hoorah

          Show
          Sam Hemelryk added a comment - Woohoo I've just commit a solution to P22. Hoorah
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited
          • P23 Final destination after backup ends should fulfill this (originally explained at MDL-22145).

          Edited: Ops, forget about this (P23) I've been performing various backups (no users or anonymised) and both are redirected to user_backup area.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited P23 Final destination after backup ends should fulfill this (originally explained at MDL-22145 ). Edited: Ops, forget about this (P23) I've been performing various backups (no users or anonymised) and both are redirected to user_backup area.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Oki, I've added all the missing dependencies (P3). Just as "normal" ones. Observing some annoying behaviors... with some settings expecting to be locked and not being... more details coming.

          Show
          Eloy Lafuente (stronk7) added a comment - Oki, I've added all the missing dependencies (P3). Just as "normal" ones. Observing some annoying behaviors... with some settings expecting to be locked and not being... more details coming.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Case 1: dependencies not working "on cascade" in mform.

          I've defined:

          activity_included dependent of section_included
          section_userinfo also dependent of section_included

          and then

          activity_userinfo dependent of section_userinfo

          Expected behavior: by unchecking section_included, all the rest (section_userinfo, activity_included and activity_userinfo) should become unchecked and disabled.

          Current behavior: section_userinfo and activity_included are disable (not unchecked) and activity_userinfo continues enabled and checked.

          Note: If I go to the confirmation stage, I see all them properly set to no and locked. So I guess it's some failure only at the mform level, not supporting "chained" changes to happen. Can this be fixed in any way? Or do I need to explicity create a new dependency to make activity_userinfo directly dependent of section_included? From a backup architecture pov sounds far from perfect, as far as "chained" dependencies work there.

          Note2: Perhaps if the chained thing is a "hard" limit in the mforms stuff, when creating it, we should make to follow all the chain-tree (down-up) and accumulate disableif rules from all the possible "ancestors" ? Of course, if they can work in a chained way (it isn't a hard limit in mforms, better).

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Case 1 : dependencies not working "on cascade" in mform. I've defined: activity_included dependent of section_included section_userinfo also dependent of section_included and then activity_userinfo dependent of section_userinfo Expected behavior: by unchecking section_included, all the rest (section_userinfo, activity_included and activity_userinfo) should become unchecked and disabled. Current behavior: section_userinfo and activity_included are disable (not unchecked) and activity_userinfo continues enabled and checked. Note: If I go to the confirmation stage, I see all them properly set to no and locked. So I guess it's some failure only at the mform level, not supporting "chained" changes to happen. Can this be fixed in any way? Or do I need to explicity create a new dependency to make activity_userinfo directly dependent of section_included? From a backup architecture pov sounds far from perfect, as far as "chained" dependencies work there. Note2: Perhaps if the chained thing is a "hard" limit in the mforms stuff, when creating it, we should make to follow all the chain-tree (down-up) and accumulate disableif rules from all the possible "ancestors" ? Of course, if they can work in a chained way (it isn't a hard limit in mforms, better).
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Case 2 : some "direct" dependencies not disabling dependent settings.

          I've defined:

          activity_included dependent to root activities setting
          activity_included is also dependent of section_included

          finally also:

          activity_userinfo is dependent of action_included

          Expected behavior: by selecting "no" in root activities setting, all the activity_included should become unchecked and disabled (directly) and all the activity_userinfo too (this indirectly).

          Current behavior: activity_userinfo settings are shown properly unchecked and disabled (the indirect one!) but activity_included (direct relation!) shows them unchecked but enabled (not locked)

          Note: This seems to be against the Case 1 above, so here we have indirect dependencies working (although between different stages). The strange point is why the activity_included settings aren't disabled.

          Note 2: Perhaps it's because of the other dependency they have (with section_included?). If so, we are back somehow to P21, so some dependencies are affecting to others improperly, unlocking things that should remain locked by another dependency.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Case 2 : some "direct" dependencies not disabling dependent settings. I've defined: activity_included dependent to root activities setting activity_included is also dependent of section_included finally also: activity_userinfo is dependent of action_included Expected behavior: by selecting "no" in root activities setting, all the activity_included should become unchecked and disabled (directly) and all the activity_userinfo too (this indirectly). Current behavior: activity_userinfo settings are shown properly unchecked and disabled (the indirect one!) but activity_included (direct relation!) shows them unchecked but enabled (not locked) Note: This seems to be against the Case 1 above, so here we have indirect dependencies working (although between different stages). The strange point is why the activity_included settings aren't disabled. Note 2: Perhaps it's because of the other dependency they have (with section_included?). If so, we are back somehow to P21, so some dependencies are affecting to others improperly, unlocking things that should remain locked by another dependency.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Apart from the cases 1 & 2, two new things, one really critical, the other less priority. Both found once security checks (MDL-22141) have been implemented.

          P24: Critical. Security checks, for users not having the 'moodle/backup:userinfo' (teachers by default), sets the 'users' setting to value = false and status = LOCKED_BY_PERMISSION. But... once the UI is shown... the setting isn't "locked" at all. Worst, teacher is able to set it back to yes (enabling a lot of dependent settings). Luckily, it breaks in schema page, but they shouldn't be able to change that setting at all. LOCKED_BY_PERMISSION should be impossible to unlock / change value. Curiously, if you go to the schema page without changing it to "yes", in that page it appears with the tiny lock icon. To reproduce, just try one backup as teacher.

          P25: Less priority. There is one new capability ( 'moodle/backup:configure' ) that specifies if the user can configure any of the backup settings or no. Form the UI POV it means that users lacking the capability should go straight to the "confirmation" stage without possibility to edit anything. Not critical, as far as, by default, all the teachers have it allowed, but once ppl start playing with the capability... it should be working as commented.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Apart from the cases 1 & 2, two new things, one really critical, the other less priority. Both found once security checks ( MDL-22141 ) have been implemented. P24 : Critical. Security checks, for users not having the 'moodle/backup:userinfo' (teachers by default), sets the 'users' setting to value = false and status = LOCKED_BY_PERMISSION. But... once the UI is shown... the setting isn't "locked" at all. Worst, teacher is able to set it back to yes (enabling a lot of dependent settings). Luckily, it breaks in schema page, but they shouldn't be able to change that setting at all. LOCKED_BY_PERMISSION should be impossible to unlock / change value. Curiously, if you go to the schema page without changing it to "yes", in that page it appears with the tiny lock icon. To reproduce, just try one backup as teacher. P25 : Less priority. There is one new capability ( 'moodle/backup:configure' ) that specifies if the user can configure any of the backup settings or no. Form the UI POV it means that users lacking the capability should go straight to the "confirmation" stage without possibility to edit anything. Not critical, as far as, by default, all the teachers have it allowed, but once ppl start playing with the capability... it should be working as commented.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Just reviewing all the pending points and cases and putting all them here
          (I've annotated them as critical, important, medium, low just in case you need to prioritize them):

          • P6 - LOW - The output singleton: That singleton was added there as a way to separate any output from process completely. And also, allow easy extension along the time (providing other visualizations of the progress, right now it's a simple "lister"). Its only relation with loggers is that we can "flag" logger actions to be also sent to output if desired but it isn't in charge of outputting logs (each logger has its own output defined, independent of the singleton).
            So, along the next days... I'll be adding log() - backup_helper::log() - calls here and there, and some of them will have the $display param enabled, hence will be sent to singleton, also, there will be some direct calls to output_controller::get_instance->output() sending other bits to the singleton.
            So, from the backup ui POV, one "list" (html/text) will be set to output (execution stage). Just that. I don't think we need any integration (ui and output don't need to "meet" the other at all). But, perhaps, instead of the current auto-redirect at the end of the stage... we should show one "continue" button, to give the user the possibility of reading the output and so on. Just that.
          • P13 - MEDIUM - Filling filename: both format and type are controller's format and type. Same for id. In the other side, users and anonymised are the values of those settings. I was thinking about to set the default for filename = ''. Then, when it's firstly "rendered" or harcoded in that stage page, fill it if empty with results from the function. Also note that that filed must be validated to be non empty, PARAM_FILENAME and end with .zip (we are missing those validations right now, my fault I didn't specified them anywhere).
          • P24 - CRITICAL - Security checks, for users not having the 'moodle/backup:userinfo' (teachers by default), sets the 'users' setting to value = false and status = LOCKED_BY_PERMISSION. But... once the UI is shown... the setting isn't "locked" at all. Worst, teacher is able to set it back to yes (enabling a lot of dependent settings). Luckily, it breaks in schema page, but they shouldn't be able to change that setting at all. LOCKED_BY_PERMISSION should be impossible to unlock / change value. Curiously, if you go to the schema page without changing it to "yes", in that page it appears with the tiny lock icon. To reproduce, just try one backup as teacher.
          • P25 - LOW - There is one new capability ( 'moodle/backup:configure' ) that specifies if the user can configure any of the backup settings or no. Form the UI POV it means that users lacking the capability should go straight to the "confirmation" stage without possibility to edit anything. Not critical, as far as, by default, all the teachers have it allowed, but once ppl start playing with the capability... it should be working as commented.
          • Case 1 - IMPORTANT - dependencies not working "on cascade" in mform.

          I've defined:

          activity_included dependent of section_included
          section_userinfo also dependent of section_included

          and then

          activity_userinfo dependent of section_userinfo

          Expected behavior: by unchecking section_included, all the rest (section_userinfo, activity_included and activity_userinfo) should become unchecked and disabled.

          Current behavior: section_userinfo and activity_included are disable (not unchecked) and activity_userinfo continues enabled and checked.

          Note: If I go to the confirmation stage, I see all them properly set to no and locked. So I guess it's some failure only at the mform level, not supporting "chained" changes to happen. Can this be fixed in any way? Or do I need to explicity create a new dependency to make activity_userinfo directly dependent of section_included? From a backup architecture pov sounds far from perfect, as far as "chained" dependencies work there.

          Note2: Perhaps if the chained thing is a "hard" limit in the mforms stuff, when creating it, we should make to follow all the chain-tree (down-up) and accumulate disableif rules from all the possible "ancestors" ? Of course, if they can work in a chained way (it isn't a hard limit in mforms, better).

          • Case 2 - IMPORTANT - some "direct" dependencies not disabling dependent settings.

          I've defined:

          activity_included dependent to root activities setting
          activity_included is also dependent of section_included

          finally also:

          activity_userinfo is dependent of action_included

          Expected behavior: by selecting "no" in root activities setting, all the activity_included should become unchecked and disabled (directly) and all the activity_userinfo too (this indirectly).

          Current behavior: activity_userinfo settings are shown properly unchecked and disabled (the indirect one!) but activity_included (direct relation!) shows them unchecked but enabled (not locked)

          Note: This seems to be against the Case 1 above, so here we have indirect dependencies working (although between different stages). The strange point is why the activity_included settings aren't disabled.

          Note 2: Perhaps it's because of the other dependency they have (with section_included?). If so, we are back somehow to P21, so some dependencies are affecting to others improperly, unlocking things that should remain locked by another dependency.

          • PMEMORY - MEDIUM - TO RESEARCH - Finally one comment that, while not important for 1-backup can have high relevance when implementing multiple backups (in the same request) like scheduled backups / category backups...

          I've noticed that, after putting all the ui machinery to work, in my test server, memory usage has grown from 10M to 15M for the same course. Worst, executing in the same script 5 backups, caused each one to raise memory 1M, and now it's near 2M (I guess this is because of the new circular references introduced that php isn't able to destroy properly and now we far more than before).

          So this is one problem to take into account when implementing multiple-backups unless we find one way to properly destroy all those references, so php will be able to gc that memory. Nor critical for now... but could become...

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Just reviewing all the pending points and cases and putting all them here (I've annotated them as critical, important, medium, low just in case you need to prioritize them): P6 - LOW - The output singleton: That singleton was added there as a way to separate any output from process completely. And also, allow easy extension along the time (providing other visualizations of the progress, right now it's a simple "lister"). Its only relation with loggers is that we can "flag" logger actions to be also sent to output if desired but it isn't in charge of outputting logs (each logger has its own output defined, independent of the singleton). So, along the next days... I'll be adding log() - backup_helper::log() - calls here and there, and some of them will have the $display param enabled, hence will be sent to singleton, also, there will be some direct calls to output_controller::get_instance->output() sending other bits to the singleton. So, from the backup ui POV, one "list" (html/text) will be set to output (execution stage). Just that. I don't think we need any integration (ui and output don't need to "meet" the other at all). But, perhaps, instead of the current auto-redirect at the end of the stage... we should show one "continue" button, to give the user the possibility of reading the output and so on. Just that. P13 - MEDIUM - Filling filename: both format and type are controller's format and type. Same for id. In the other side, users and anonymised are the values of those settings. I was thinking about to set the default for filename = ''. Then, when it's firstly "rendered" or harcoded in that stage page, fill it if empty with results from the function. Also note that that filed must be validated to be non empty, PARAM_FILENAME and end with .zip (we are missing those validations right now, my fault I didn't specified them anywhere). P24 - CRITICAL - Security checks, for users not having the 'moodle/backup:userinfo' (teachers by default), sets the 'users' setting to value = false and status = LOCKED_BY_PERMISSION. But... once the UI is shown... the setting isn't "locked" at all. Worst, teacher is able to set it back to yes (enabling a lot of dependent settings). Luckily, it breaks in schema page, but they shouldn't be able to change that setting at all. LOCKED_BY_PERMISSION should be impossible to unlock / change value. Curiously, if you go to the schema page without changing it to "yes", in that page it appears with the tiny lock icon. To reproduce, just try one backup as teacher. P25 - LOW - There is one new capability ( 'moodle/backup:configure' ) that specifies if the user can configure any of the backup settings or no. Form the UI POV it means that users lacking the capability should go straight to the "confirmation" stage without possibility to edit anything. Not critical, as far as, by default, all the teachers have it allowed, but once ppl start playing with the capability... it should be working as commented. Case 1 - IMPORTANT - dependencies not working "on cascade" in mform. I've defined: activity_included dependent of section_included section_userinfo also dependent of section_included and then activity_userinfo dependent of section_userinfo Expected behavior: by unchecking section_included, all the rest (section_userinfo, activity_included and activity_userinfo) should become unchecked and disabled. Current behavior: section_userinfo and activity_included are disable (not unchecked) and activity_userinfo continues enabled and checked. Note: If I go to the confirmation stage, I see all them properly set to no and locked. So I guess it's some failure only at the mform level, not supporting "chained" changes to happen. Can this be fixed in any way? Or do I need to explicity create a new dependency to make activity_userinfo directly dependent of section_included? From a backup architecture pov sounds far from perfect, as far as "chained" dependencies work there. Note2: Perhaps if the chained thing is a "hard" limit in the mforms stuff, when creating it, we should make to follow all the chain-tree (down-up) and accumulate disableif rules from all the possible "ancestors" ? Of course, if they can work in a chained way (it isn't a hard limit in mforms, better). Case 2 - IMPORTANT - some "direct" dependencies not disabling dependent settings. I've defined: activity_included dependent to root activities setting activity_included is also dependent of section_included finally also: activity_userinfo is dependent of action_included Expected behavior: by selecting "no" in root activities setting, all the activity_included should become unchecked and disabled (directly) and all the activity_userinfo too (this indirectly). Current behavior: activity_userinfo settings are shown properly unchecked and disabled (the indirect one!) but activity_included (direct relation!) shows them unchecked but enabled (not locked) Note: This seems to be against the Case 1 above, so here we have indirect dependencies working (although between different stages). The strange point is why the activity_included settings aren't disabled. Note 2: Perhaps it's because of the other dependency they have (with section_included?). If so, we are back somehow to P21, so some dependencies are affecting to others improperly, unlocking things that should remain locked by another dependency. PMEMORY - MEDIUM - TO RESEARCH - Finally one comment that, while not important for 1-backup can have high relevance when implementing multiple backups (in the same request) like scheduled backups / category backups... I've noticed that, after putting all the ui machinery to work, in my test server, memory usage has grown from 10M to 15M for the same course. Worst, executing in the same script 5 backups, caused each one to raise memory 1M, and now it's near 2M (I guess this is because of the new circular references introduced that php isn't able to destroy properly and now we far more than before). So this is one problem to take into account when implementing multiple-backups unless we find one way to properly destroy all those references, so php will be able to gc that memory. Nor critical for now... but could become...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Great, all the critical / important remaining issued have been addressed by Sam (P13, P24, Case1, Case2).

          Just reposting here the pending ones (all low-priority):

          P6 - LOW - The output singleton

          That singleton was added there as a way to separate any output from process completely. And also, allow easy extension along the time (providing other visualizations of the progress, right now it's a simple "lister"). Its only relation with loggers is that we can "flag" logger actions to be also sent to output if desired but it isn't in charge of outputting logs (each logger has its own output defined, independent of the singleton).
          So, along the next days... I'll be adding log() - backup_helper::log() - calls here and there, and some of them will have the $display param enabled, hence will be sent to singleton, also, there will be some direct calls to output_controller::get_instance->output() sending other bits to the singleton.
          So, from the backup ui POV, one "list" (html/text) will be set to output (execution stage). Just that. I don't think we need any integration (ui and output don't need to "meet" the other at all). But, perhaps, instead of the current auto-redirect at the end of the stage... we should show one "continue" button, to give the user the possibility of reading the output and so on. Just that.

          P25 - LOW - new capability ( 'moodle/backup:configure' ) integration in UI

          There is one new capability ( 'moodle/backup:configure' ) that specifies if the user can configure any of the backup settings or no. Form the UI POV it means that users lacking the capability should go straight to the "confirmation" stage without possibility to edit anything. Not critical, as far as, by default, all the teachers have it allowed, but once ppl start playing with the capability... it should be working as commented.

          PMEMORY - MEDIUM - TO RESEARCH - Memory usage

          Finally one comment that, while not important for 1-backup can have high relevance when implementing multiple backups (in the same request) like scheduled backups / category backups...

          I've noticed that, after putting all the ui machinery to work, in my test server, memory usage has grown from 10M to 15M for the same course. Worst, executing in the same script 5 backups, caused each one to raise memory 1M, and now it's near 2M (I guess this is because of the new circular references introduced that php isn't able to destroy properly and now we far more than before).

          So this is one problem to take into account when implementing multiple-backups unless we find one way to properly destroy all those references, so php will be able to gc that memory. Nor critical for now... but could become...

          Show
          Eloy Lafuente (stronk7) added a comment - Great, all the critical / important remaining issued have been addressed by Sam (P13, P24, Case1, Case2). Just reposting here the pending ones (all low-priority): P6 - LOW - The output singleton That singleton was added there as a way to separate any output from process completely. And also, allow easy extension along the time (providing other visualizations of the progress, right now it's a simple "lister"). Its only relation with loggers is that we can "flag" logger actions to be also sent to output if desired but it isn't in charge of outputting logs (each logger has its own output defined, independent of the singleton). So, along the next days... I'll be adding log() - backup_helper::log() - calls here and there, and some of them will have the $display param enabled, hence will be sent to singleton, also, there will be some direct calls to output_controller::get_instance->output() sending other bits to the singleton. So, from the backup ui POV, one "list" (html/text) will be set to output (execution stage). Just that. I don't think we need any integration (ui and output don't need to "meet" the other at all). But, perhaps, instead of the current auto-redirect at the end of the stage... we should show one "continue" button, to give the user the possibility of reading the output and so on. Just that. P25 - LOW - new capability ( 'moodle/backup:configure' ) integration in UI There is one new capability ( 'moodle/backup:configure' ) that specifies if the user can configure any of the backup settings or no. Form the UI POV it means that users lacking the capability should go straight to the "confirmation" stage without possibility to edit anything. Not critical, as far as, by default, all the teachers have it allowed, but once ppl start playing with the capability... it should be working as commented. PMEMORY - MEDIUM - TO RESEARCH - Memory usage Finally one comment that, while not important for 1-backup can have high relevance when implementing multiple backups (in the same request) like scheduled backups / category backups... I've noticed that, after putting all the ui machinery to work, in my test server, memory usage has grown from 10M to 15M for the same course. Worst, executing in the same script 5 backups, caused each one to raise memory 1M, and now it's near 2M (I guess this is because of the new circular references introduced that php isn't able to destroy properly and now we far more than before). So this is one problem to take into account when implementing multiple-backups unless we find one way to properly destroy all those references, so php will be able to gc that memory. Nor critical for now... but could become...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Just found a new one:

          P26 - IMPORTANT - backup filename blanked on non-ui backups

          Jerome detected some minutes ago that his HUB backups had suddenly stopped working. I traced down the cause to be the filename setting to be blank (empty). I've tried to fix it directly settings / ui code but failed, as far as it was urgent I just added one quick hack in the backup-storer, so if it detects blank filenames... they are recalculated one the fly (we have all the info in that part of the backup). The hack is this:

          http://cvs.moodle.org/moodle/backup/util/helper/backup_helper.class.php?r1=1.8&r2=1.9

          To reproduce the problem... just comment that hack and run this code (that should be valid):

          $bc = new backup_controller(backup::TYPE_1COURSE, $courseid, backup::FORMAT_MOODLE,
                          backup::INTERACTIVE_YES, backup::MODE_HUB, $USER->id);
          $bc->finish_ui();
          $bc->execute_plan();
          $backup = $bc->get_results();
          $backupfile = $backup['backup_destination'];
          

          it breaks badly without the hack (and cause is filename being empty.

          Any backup (no matter of its UI, or interactivity should end with the filename properly set). In fact I was expecting, for Jerome's case, to be getting "old" backup.zip filenames, as far as he isn't using the UI at all. The point that surprised me was to be getting blank filenames instead, so at some point, the UI is blanking the name but not filling it properly?

          Not critical, as far as the hack temporary fixes it, but to have solved asap (np after release).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Just found a new one: P26 - IMPORTANT - backup filename blanked on non-ui backups Jerome detected some minutes ago that his HUB backups had suddenly stopped working. I traced down the cause to be the filename setting to be blank (empty). I've tried to fix it directly settings / ui code but failed, as far as it was urgent I just added one quick hack in the backup-storer, so if it detects blank filenames... they are recalculated one the fly (we have all the info in that part of the backup). The hack is this: http://cvs.moodle.org/moodle/backup/util/helper/backup_helper.class.php?r1=1.8&r2=1.9 To reproduce the problem... just comment that hack and run this code (that should be valid): $bc = new backup_controller(backup::TYPE_1COURSE, $courseid, backup::FORMAT_MOODLE, backup::INTERACTIVE_YES, backup::MODE_HUB, $USER->id); $bc->finish_ui(); $bc->execute_plan(); $backup = $bc->get_results(); $backupfile = $backup['backup_destination']; it breaks badly without the hack (and cause is filename being empty. Any backup (no matter of its UI, or interactivity should end with the filename properly set). In fact I was expecting, for Jerome's case, to be getting "old" backup.zip filenames, as far as he isn't using the UI at all. The point that surprised me was to be getting blank filenames instead, so at some point, the UI is blanking the name but not filling it properly? Not critical, as far as the hack temporary fixes it, but to have solved asap (np after release). Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Consider P13 reopened, as far as it seems related to the newly added P26 and I've detected that such field isn't being checked to be:

          • mandatory
          • valid PARAM_FILENAME
          • ended with .zip

          So right now users can introduce anything there, that could lead to annoying results / potential exploits. Needs validation to happen.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Consider P13 reopened, as far as it seems related to the newly added P26 and I've detected that such field isn't being checked to be: mandatory valid PARAM_FILENAME ended with .zip So right now users can introduce anything there, that could lead to annoying results / potential exploits. Needs validation to happen. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Sam,

          just adding here some links pointing to some comments in MDL-22144 - the output, that are related to this - the UI. Olli posted them there because I suggested that to him wrongly, as far as they belong to here. They aren't critical (so I'd prioritize any of the pending ones above) but all them seem to be good suggestions to be considered IMO:

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Sam, just adding here some links pointing to some comments in MDL-22144 - the output, that are related to this - the UI. Olli posted them there because I suggested that to him wrongly, as far as they belong to here. They aren't critical (so I'd prioritize any of the pending ones above) but all them seem to be good suggestions to be considered IMO: http://tracker.moodle.org/browse/MDL-22144?focusedCommentId=84386&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_84386 http://tracker.moodle.org/browse/MDL-22144?focusedCommentId=84387&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_84387 Ciao
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I've just attached a patch that refactors the way the moodleform bridges were being utilised, tidies up the the filename validation, and implements many of the ideas Olli mentioned on MDL-22144.
          Changes are as follows:

          • backup moodleforms for specific stages are now cached between stage processing and display making it possible to leave all validation up to the bridge.
          • The backup stage now relies entirely on the backup controllers save, load, and check methods + dependency checks rather then previous stage processing to ensure accuracy.
          • filename is now a required field and validated to ensure it ends with .zip and is a valid filename.
          • There is now a previous button for backup stages after the initial and before the complete.
          • The progress bar previous stages are now links as well allowing the user to jump to previous stages (not linked on complete of course).
          • The root settings are only shown on the schema stage now if they were changed because of dependency.
          • Stages are now numbered in the progress bar.
          • The complete page now shows a notice that the backup was complete and provides a continue button to view the backup file.
          • Adds administration blocks to the backup pages.

          Having discussed this with Eloy I will hold off committing this patch until after PR1 is rolled.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I've just attached a patch that refactors the way the moodleform bridges were being utilised, tidies up the the filename validation, and implements many of the ideas Olli mentioned on MDL-22144 . Changes are as follows: backup moodleforms for specific stages are now cached between stage processing and display making it possible to leave all validation up to the bridge. The backup stage now relies entirely on the backup controllers save, load, and check methods + dependency checks rather then previous stage processing to ensure accuracy. filename is now a required field and validated to ensure it ends with .zip and is a valid filename. There is now a previous button for backup stages after the initial and before the complete. The progress bar previous stages are now links as well allowing the user to jump to previous stages (not linked on complete of course). The root settings are only shown on the schema stage now if they were changed because of dependency. Stages are now numbered in the progress bar. The complete page now shows a notice that the backup was complete and provides a continue button to view the backup file. Adds administration blocks to the backup pages. Having discussed this with Eloy I will hold off committing this patch until after PR1 is rolled. Cheers Sam
          Hide
          Olli Savolainen added a comment -

          Great stuff Sam!

          I commented on stuff on the dev channel before realizing you had listed the above fixes here already.

          Sorry for the improvement suggestions coming from me so gradually.

          The tricky thing about the previous button (yours I have not seen yet since I currently rely on qa.moodle.net only) implementation wise is that also the changes to the form you come back to earlier steps from, need to be preserved (so it is not the same as browser back button). Links in progress bar are actually not required by the wizard patter, but I guess they do not hurt. Anyway, if there are links in the progress bar, they need to preserve changes of later stages too (so I guess you need some AJAX magic?) - you are not allowed to lose user changes.

          14:29:52 <tsala_> does it need any help popups do you think?
          14:35:10 <pilpi> I had not really taken a good look at the contents of the wizard as a whole, just the wizard mechanics so as frustrating as it is I believe some explanation is still needed in the ui itself...
          14:35:37 <pilpi> Helen, on the first step there is no meta information about what all the "backup settings" actually mean. users: yes/no - a guess is that users will wonder "what is it about users that I need to answer yes/no to?".

          <pilpi> some of these are settings of what to include, and some direct the wizard's behaviour on all of them
          14:35:43 <pilpi> I guess
          14:36:42 <pilpi> anyway, that separation would probably be more visible

          14:37:10 <pilpi> in addition to simple explanation of what the items on the first step actually are

          Show
          Olli Savolainen added a comment - Great stuff Sam! I commented on stuff on the dev channel before realizing you had listed the above fixes here already. Sorry for the improvement suggestions coming from me so gradually. The tricky thing about the previous button (yours I have not seen yet since I currently rely on qa.moodle.net only) implementation wise is that also the changes to the form you come back to earlier steps from, need to be preserved (so it is not the same as browser back button). Links in progress bar are actually not required by the wizard patter, but I guess they do not hurt. Anyway, if there are links in the progress bar, they need to preserve changes of later stages too (so I guess you need some AJAX magic?) - you are not allowed to lose user changes. 14:29:52 <tsala_> does it need any help popups do you think? 14:35:10 <pilpi> I had not really taken a good look at the contents of the wizard as a whole, just the wizard mechanics so as frustrating as it is I believe some explanation is still needed in the ui itself... 14:35:37 <pilpi> Helen, on the first step there is no meta information about what all the "backup settings" actually mean. users: yes/no - a guess is that users will wonder "what is it about users that I need to answer yes/no to?". <pilpi> some of these are settings of what to include, and some direct the wizard's behaviour on all of them 14:35:43 <pilpi> I guess 14:36:42 <pilpi> anyway, that separation would probably be more visible 14:37:10 <pilpi> in addition to simple explanation of what the items on the first step actually are
          Hide
          Olli Savolainen added a comment -

          After Eloy's comment about the form being build dynamically (and about it adding complexity to create a new fieldset):

          Maybe just set the fieldset label to "items to include" and label the anonymize setting "personal info" after all (and somehow make the degree of anonymization that happens on unchecking, clear).

          Show
          Olli Savolainen added a comment - After Eloy's comment about the form being build dynamically (and about it adding complexity to create a new fieldset): Maybe just set the fieldset label to "items to include" and label the anonymize setting "personal info" after all (and somehow make the degree of anonymization that happens on unchecking, clear).
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Sam, just have looked and tested your last patch:

          1) Changes look ok (better organized while keeping functionality).
          2) The most I review it, Olli and Sam, the most I think we should make the wizard a "forward only" wizard. I'm sure that "previous" button is going to cause us problems here and there as far as it makes dependencies more and more complex. For example, we have one dependency instructing to the system so, if "users" is set to no, all the "include userinfo" in second stage will be disabled/set to no. But that's one "one direction and one time" dependency! It doesn't work if we go back and re-enable "users" to yes. And that's just one simple example. So please consider it but my vote goes to forbid "previous". There is no problem "restarting" the backup at all, but the previous option can really be problematic. -1 for this.
          3) Sam, I just tried deleting the ".zip" from the name... and backup happened! (same leaving the field completely blanck). So i guess there is someplace where we are missing the validation.
          4) Right now, Sam and Olli, we are using the setting name (exactly as it's named in backup code, as labels in the form). I think we can change it once and for all so we get them from 'backup' lang file without problems. Once done, we can name label them as we want. +1 for this.

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Sam, just have looked and tested your last patch: 1) Changes look ok (better organized while keeping functionality). 2) The most I review it, Olli and Sam, the most I think we should make the wizard a "forward only" wizard. I'm sure that "previous" button is going to cause us problems here and there as far as it makes dependencies more and more complex. For example, we have one dependency instructing to the system so, if "users" is set to no, all the "include userinfo" in second stage will be disabled/set to no. But that's one "one direction and one time" dependency! It doesn't work if we go back and re-enable "users" to yes. And that's just one simple example. So please consider it but my vote goes to forbid "previous". There is no problem "restarting" the backup at all, but the previous option can really be problematic. -1 for this. 3) Sam, I just tried deleting the ".zip" from the name... and backup happened! (same leaving the field completely blanck). So i guess there is someplace where we are missing the validation. 4) Right now, Sam and Olli, we are using the setting name (exactly as it's named in backup code, as labels in the form). I think we can change it once and for all so we get them from 'backup' lang file without problems. Once done, we can name label them as we want. +1 for this.
          Hide
          Olli Savolainen added a comment -

          Just took a look at the version at qa.moodle.net.
          What is the semantic difference between "locked" and greyed out?
          Why is an item that a user has already selected previously in the wizard "locked" when they were just greyed out in a previous step?
          If an item is not editable because you are giving a summary to the user, it does not make sense that you say it is "locked". It is simply "shown" in a summary that is by definition not editable.

          2. Ok. This is likely not fatal, since there are so few steps anyway.

          Still: I delivered the spec for how a wizard works and how a backup wizard looks, eight months ago, and provided a sample implementation of it (PEAR Quickform_controller - even if you do not like it, there is a lot you can use in any case), so I am sad we can not implement the basic functionality of a wizard that is definitely expected by users. If the users' goals do not drive the requirements set for the functionality, then what is it we are trying to do? I would love to do design with you beforehand, but as I am not included in the process of defining the requirements for the functionality and the process is essentially invisible to me, I have no option but trying to put out fires afterwards.

          Maybe cancelling the wizard in this case should take the user to the first step, with a box notifying the user: "Backup cancelled."
          Knowing if this confuses users users would require testing, but the other option would be to take the user to the exact page where the user clicked on the link to Backup (not always to course front page) - we could get that from the http referer perhaps or give it as a parameter to backup.

          Show
          Olli Savolainen added a comment - Just took a look at the version at qa.moodle.net. What is the semantic difference between "locked" and greyed out? Why is an item that a user has already selected previously in the wizard "locked" when they were just greyed out in a previous step? If an item is not editable because you are giving a summary to the user, it does not make sense that you say it is "locked". It is simply "shown" in a summary that is by definition not editable. 2. Ok. This is likely not fatal, since there are so few steps anyway. Still: I delivered the spec for how a wizard works and how a backup wizard looks, eight months ago, and provided a sample implementation of it (PEAR Quickform_controller - even if you do not like it, there is a lot you can use in any case), so I am sad we can not implement the basic functionality of a wizard that is definitely expected by users. If the users' goals do not drive the requirements set for the functionality, then what is it we are trying to do? I would love to do design with you beforehand, but as I am not included in the process of defining the requirements for the functionality and the process is essentially invisible to me, I have no option but trying to put out fires afterwards. Maybe cancelling the wizard in this case should take the user to the first step, with a box notifying the user: "Backup cancelled." Knowing if this confuses users users would require testing, but the other option would be to take the user to the exact page where the user clicked on the link to Backup (not always to course front page) - we could get that from the http referer perhaps or give it as a parameter to backup.
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          First up Eloy, I have commit this patch this morning.
          I made a couple of minor adjustments to the mforms validation and tested the validation successfully several times.
          The previous button, plus links have been left in presently as I tested changing values in several places and in all circumstances the dependencies enforced correctly. However I certainly see that this is one area where confusion may arise as to why settings are being changed when jumping back stage(s), I think we will need to watch the use of this functionality and see whether it raises issues.
          In regards for 4. YES +1 for on moving these to backup lang file, will have to decide how to go about it so will aim to raise it in DEV chat when I spot you online next.

          Olli, my apologies for not including you on the design process for this issue. In this case there was no real design spec, there was an urgent requirement to get a working interface created in time for Moodle 2.0 preview release, and the interface that we have now was simply the most straightforward interface I could create that met the needs of the fantastic backup system that Eloy has created. In short do not regret attempting to put out fires at this point, that is the only away to progress.

          The `locked` vs. greyed out items in the backup could certainly be changed, showing the lock in the summary is not needed and instead should be shown against items that have been disabled through interstage dependency (only where the item has an enforced dependency to a setting from a previous stage), I will look to clean that up when I get a moment.

          As for the cancel button redirecting the wizard to the page from which they clicked on backup, it is an option that could be considered. I would rather not pass it along as a parameter of the initial request, but trying to pick it up on the initial request may be an option. Eloy what do you think about this? you are probably more familiar with what is done in Moodle here than what I am?

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, First up Eloy, I have commit this patch this morning. I made a couple of minor adjustments to the mforms validation and tested the validation successfully several times. The previous button, plus links have been left in presently as I tested changing values in several places and in all circumstances the dependencies enforced correctly. However I certainly see that this is one area where confusion may arise as to why settings are being changed when jumping back stage(s), I think we will need to watch the use of this functionality and see whether it raises issues. In regards for 4. YES +1 for on moving these to backup lang file, will have to decide how to go about it so will aim to raise it in DEV chat when I spot you online next. Olli, my apologies for not including you on the design process for this issue. In this case there was no real design spec, there was an urgent requirement to get a working interface created in time for Moodle 2.0 preview release, and the interface that we have now was simply the most straightforward interface I could create that met the needs of the fantastic backup system that Eloy has created. In short do not regret attempting to put out fires at this point, that is the only away to progress. The `locked` vs. greyed out items in the backup could certainly be changed, showing the lock in the summary is not needed and instead should be shown against items that have been disabled through interstage dependency (only where the item has an enforced dependency to a setting from a previous stage), I will look to clean that up when I get a moment. As for the cancel button redirecting the wizard to the page from which they clicked on backup, it is an option that could be considered. I would rather not pass it along as a parameter of the initial request, but trying to pick it up on the initial request may be an option. Eloy what do you think about this? you are probably more familiar with what is done in Moodle here than what I am? Cheers Sam
          Hide
          Olli Savolainen added a comment -

          Thanks, Sam.

          The above sounded too much like an accusation, which I did not intend it to be. As much as I would like to be involved in these processes, Moodle is a big application and explicitly including users in the design phase is just not there yet, so no point for me in pointing fingers at one effort. My resources for working on things have certainly been limited, and what I can do will always be less than what Moodle really needs, so shouting that I want to get included carries the risk that people will actually start doing that, and I won't be able to help.

          What I wanted to say is simply that I see a need to think about the processes we go along, however informal those processes may be.

          As I mentioned, I am not sure which solution for the cancel button would work. Seems that placing the user back to where they came from is far if they wanted to just start the wizard over, and if the button takes to the start of the wizard and resets the wizard then getting back to where they were when they clicked 'backup' is hard. How many realistic (common) places are imaginable where users can click a link to get to backup? If not many of them are common, what I suggest may just be extra work with little benefit. So no clear solution here without user research, and the whole question seems less than critical.

          Thanks Sam for the great attitude that shows in your response. Keep up the good work!

          Show
          Olli Savolainen added a comment - Thanks, Sam. The above sounded too much like an accusation, which I did not intend it to be. As much as I would like to be involved in these processes, Moodle is a big application and explicitly including users in the design phase is just not there yet, so no point for me in pointing fingers at one effort. My resources for working on things have certainly been limited, and what I can do will always be less than what Moodle really needs, so shouting that I want to get included carries the risk that people will actually start doing that, and I won't be able to help. What I wanted to say is simply that I see a need to think about the processes we go along, however informal those processes may be. As I mentioned, I am not sure which solution for the cancel button would work. Seems that placing the user back to where they came from is far if they wanted to just start the wizard over, and if the button takes to the start of the wizard and resets the wizard then getting back to where they were when they clicked 'backup' is hard. How many realistic (common) places are imaginable where users can click a link to get to backup? If not many of them are common, what I suggest may just be extra work with little benefit. So no clear solution here without user research, and the whole question seems less than critical. Thanks Sam for the great attitude that shows in your response. Keep up the good work!
          Hide
          Olli Savolainen added a comment -

          Anyway, hope to get the wizard in top shape before using it in other places it is needed, i.e. installation. (I can imagine there are technical limitations for why applying the current wizard inside Moodle might not be easy for installation which is in a sense "pre-moodle", but nonetheless the unpredictability of the installation wizard currently should be addressed. Gallery 2 (http://gallery.menalto.com) regardless of what you otherwise think of that app, has a great installation wizard, in addition to wordpress - though of course Moodle installation is perhaps less for end users than with those web apps so the priority for installation usability may be lower.

          Show
          Olli Savolainen added a comment - Anyway, hope to get the wizard in top shape before using it in other places it is needed, i.e. installation. (I can imagine there are technical limitations for why applying the current wizard inside Moodle might not be easy for installation which is in a sense "pre-moodle", but nonetheless the unpredictability of the installation wizard currently should be addressed. Gallery 2 ( http://gallery.menalto.com ) regardless of what you otherwise think of that app, has a great installation wizard, in addition to wordpress - though of course Moodle installation is perhaps less for end users than with those web apps so the priority for installation usability may be lower.
          Hide
          Olli Savolainen added a comment -

          Just took a look at the current version on qa.moodle.net. So beautiful, brings a tear to my eye .

          Thanks for all your work Sam and Eloy, great to see this happen.

          Show
          Olli Savolainen added a comment - Just took a look at the current version on qa.moodle.net. So beautiful, brings a tear to my eye . Thanks for all your work Sam and Eloy, great to see this happen.
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          Just realised I hadn't marked this issue as resolved. At this point I really think it is the case.
          Feel free to reopen if there is something I have missed.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, Just realised I hadn't marked this issue as resolved. At this point I really think it is the case. Feel free to reopen if there is something I have missed. Cheers Sam
          Hide
          Martin Dougiamas added a comment -

          One little thing I noticed is that it seems to be printing the module names "raw" and not using get_string to print them. eg "label" instead of "Label" and so on.

          Show
          Martin Dougiamas added a comment - One little thing I noticed is that it seems to be printing the module names "raw" and not using get_string to print them. eg "label" instead of "Label" and so on.
          Hide
          Sam Hemelryk added a comment -

          Hmm that is a timely reminder thank you Martin, currently all settings print their setting name rather than a label (or just meaningful text).
          This means that in the case of activities you get the raw name, however it isn't just activities it will be the same for all settings the initial general settings being one prominent example.
          What are your thoughts on this Eloy?
          For me immediately three options spring to mind:

          1. Add a label property to the base_setting class + constructor: is sort of intertwining the settings and UI code more and would require to review of all setting implementations that have already been done.
          2. Add calls for every setting: $setting->get_ui()->set_label(get_string(...)); this however is really just as much work as option 1 and makes a little less sense.
          3. Modify the backup_ui classes adding some smarts to lookup specific strings and fail over to the backup lang file.

          I am thinking options 1 or 3 myself, I personally like option 1 but 3 may be a more immediate solution. Alternatively I may have missed something obvious so let me know what you think.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hmm that is a timely reminder thank you Martin, currently all settings print their setting name rather than a label (or just meaningful text). This means that in the case of activities you get the raw name, however it isn't just activities it will be the same for all settings the initial general settings being one prominent example. What are your thoughts on this Eloy? For me immediately three options spring to mind: Add a label property to the base_setting class + constructor: is sort of intertwining the settings and UI code more and would require to review of all setting implementations that have already been done. Add calls for every setting: $setting->get_ui()->set_label(get_string(...)); this however is really just as much work as option 1 and makes a little less sense. Modify the backup_ui classes adding some smarts to lookup specific strings and fail over to the backup lang file. I am thinking options 1 or 3 myself, I personally like option 1 but 3 may be a more immediate solution. Alternatively I may have missed something obvious so let me know what you think. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Need to find a solution for the setting labels

          Show
          Sam Hemelryk added a comment - Need to find a solution for the setting labels
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Cannot we simply achieve this like it's done for the 'filename' setting?

          $filename = new backup_filename_setting('filename', base_setting::IS_FILENAME, 'backup.zip');
          $filename->set_ui(get_string('filename', 'backup'), 'backup.zip', array('size'=>50));
          

          Or, being more specific:

          $setting->get_ui()->set_label(get_string('xxx','yyy'));
          

          That for all the settings needing proper labeling (I think only "root" ones - 1st stage) will do the job, without introducing any new params, just using current available "API".

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Cannot we simply achieve this like it's done for the 'filename' setting? $filename = new backup_filename_setting('filename', base_setting::IS_FILENAME, 'backup.zip'); $filename->set_ui(get_string('filename', 'backup'), 'backup.zip', array('size'=>50)); Or, being more specific: $setting->get_ui()->set_label(get_string('xxx','yyy')); That for all the settings needing proper labeling (I think only "root" ones - 1st stage) will do the job, without introducing any new params, just using current available "API". Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Sam,

          I'm going to be out next 2 weeks (will connect intermittently anyway). Reviewing the list of pending tasks... there is the one related with the export/import UI. It sounds to me a lot that we already commented (on jabber) about that, viewing the main differences and so on.

          But I've been unable to find it here in the meta-bug? Do you have notes/plan about that? Do we need to discuss about it and create an new subtask?

          Basically it's one backup and one restore being executed in chain, with only the backup having a configuration (restore will import all the things included in backup) and with some things "prevented" like userinfo a others.

          I think we should ignite this asap.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Sam, I'm going to be out next 2 weeks (will connect intermittently anyway). Reviewing the list of pending tasks... there is the one related with the export/import UI. It sounds to me a lot that we already commented (on jabber) about that, viewing the main differences and so on. But I've been unable to find it here in the meta-bug? Do you have notes/plan about that? Do we need to discuss about it and create an new subtask? Basically it's one backup and one restore being executed in chain, with only the backup having a configuration (restore will import all the things included in backup) and with some things "prevented" like userinfo a others. I think we should ignite this asap. Ciao
          Hide
          Sam Hemelryk added a comment -

          Hi Eloy,

          I'm marking this issue as resolved again now, the strings were sorted out at the same time I fixed up the restore UI strings.

          In regards to the course import UI I don't believe there is a tracker issue for it yet so I will create a new subtask now and move discussion there. Talk to you about that one soon

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Eloy, I'm marking this issue as resolved again now, the strings were sorted out at the same time I fixed up the restore UI strings. In regards to the course import UI I don't believe there is a tracker issue for it yet so I will create a new subtask now and move discussion there. Talk to you about that one soon Cheers Sam

            People

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

              Dates

              • Created:
                Updated:
                Resolved: