Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.2
    • Component/s: Feedback
    • Labels:
      None
    • Testing Instructions:
      Hide

      First you have to give a user the capability “createpublictemplate” and if the user should be able to delete a public template so also the capability “deletetemplate”. Both capabilities must be given for the system-context. I have done this by creating a new empty role with just this two capabilities and have a user assigned at “Assign system roles”

      The user should also have the capabilities like a teacher on a course-context so that the user can create/modify a feedback.

      Login as that user

      Create a feedback in any course

      Insert one or more question-items but at least one label with an image

      Go to the Templates-tab. You should see the checkbox “public”.

      Check the box "public", put a name in the textbox and safe a new template

      The dropdownbox should now show this new template grouped by “public”

      Safe a template again but without checking the box "public". This template should be listed grouped by “course”.

      Click on “Delete template…” All templates are grouped by “course” and “public”

      Login as any editingteacher and go into a course.

      Create a new empty feedback.

      Go to the Templates-tab.

      In the dropdownbox the public template should be shown.

      Select this template.

      Now the preview of the template is shown and also the included image!

      Click on "Save changes" to use this template for the feedback.

      You will be redirected to the "Edit question" area.

      There should be shown the questions from template including the image!

      Show
      First you have to give a user the capability “createpublictemplate” and if the user should be able to delete a public template so also the capability “deletetemplate”. Both capabilities must be given for the system-context. I have done this by creating a new empty role with just this two capabilities and have a user assigned at “Assign system roles” The user should also have the capabilities like a teacher on a course-context so that the user can create/modify a feedback. Login as that user Create a feedback in any course Insert one or more question-items but at least one label with an image Go to the Templates-tab. You should see the checkbox “public”. Check the box "public", put a name in the textbox and safe a new template The dropdownbox should now show this new template grouped by “public” Safe a template again but without checking the box "public". This template should be listed grouped by “course”. Click on “Delete template…” All templates are grouped by “course” and “public” Login as any editingteacher and go into a course. Create a new empty feedback. Go to the Templates-tab. In the dropdownbox the public template should be shown. Select this template. Now the preview of the template is shown and also the included image! Click on "Save changes" to use this template for the feedback. You will be redirected to the "Edit question" area. There should be shown the questions from template including the image!
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-19488_master_wip
    • Rank:
      15589

      Description

      allow the creation of a "Public" feedback to "join/re-use/populate" it from different instance of the module from different courses

        Issue Links

          Activity

          Hide
          Paul Vaughan added a comment -

          Could 'Affects Version/s' be updated to reflect 2.1 please? We had this feature in 1.9, would be useful to have it again in 2.1.

          Show
          Paul Vaughan added a comment - Could 'Affects Version/s' be updated to reflect 2.1 please? We had this feature in 1.9, would be useful to have it again in 2.1.
          Hide
          Andreas Grabs added a comment -

          Hi Paul,
          I assume you mean the public template feature. I can implement this feature with a small limitation.
          Public templates only can be created on feedbacks which are located on the frontpage.
          Best regards Andreas

          Show
          Andreas Grabs added a comment - Hi Paul, I assume you mean the public template feature. I can implement this feature with a small limitation. Public templates only can be created on feedbacks which are located on the frontpage. Best regards Andreas
          Hide
          Paul Vaughan added a comment -

          Hi Andreas. Yes, that's the one. We've had a behind-the-scenes report since 2008 which collates the results from precicely-named Feedback instances, so this will be very useful.

          Thanks,

          Paul.

          Show
          Paul Vaughan added a comment - Hi Andreas. Yes, that's the one. We've had a behind-the-scenes report since 2008 which collates the results from precicely-named Feedback instances, so this will be very useful. Thanks, Paul.
          Hide
          Martin Dougiamas added a comment -

          Note that we have a policy of not introducing new core features in stable branches.

          Show
          Martin Dougiamas added a comment - Note that we have a policy of not introducing new core features in stable branches.
          Hide
          Sam Hemelryk added a comment -

          Hi Andreas,

          I'm send this back round this week sorry so that the following things can be tidied up before this gets integrated.

          • delete_template.php: The public suffix should be created as a string to which the template name is passed.
          • $CFG->frontpage is a setting that controls what gets displayed on the front page. There are a couple of uses introduced by this patch that I don't think are right. Leads to:

            Debug info: ERROR: invalid input syntax for integer: "0,1"
            SELECT * FROM mdl_feedback_template WHERE course = $1 AND ispublic = $2 ORDER BY name
            [array (
            0 => '0,1',
            1 => 1,
            )]
            Stack trace:
            • line 394 of /lib/dml/moodle_database.php: dml_read_exception thrown
            • line 232 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end()
            • line 678 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end()
            • line 1122 of /lib/dml/moodle_database.php: call to pgsql_native_moodle_database->get_records_sql()
            • line 1071 of /lib/dml/moodle_database.php: call to moodle_database->get_records_select()
            • line 1231 of /mod/feedback/lib.php: call to moodle_database->get_records()
            • line 81 of /mod/feedback/edit_form.php: call to feedback_get_template_list()
            • line 137 of /mod/feedback/edit.php: call to feedback_edit_use_template_form->set_form_elements()

          • Are we sure we only want to allow people to create public templates from feedback instances on the frontpage? Given that we have the capability for it couldn't we just rely on that?
          • When preparing the feedback edit form I think it would be better if we grouped options rather than prefixing the template name with an *. Alternatively if its decided to continue using the * we should probably have a string for it so that should it have a different meaning or understanding in other languages they can change it.

          Other than those this looks very good! Presently because of the above error I can't get too far into playing with it in the interface and I look forward to seeing that!

          In regards to which branches this will be applied to because it is a new feature (despite having all the hooks + capability there) it will only go into master. Perhaps if there is a strong enough case it can be backported to MOODLE_21_STABLE however that is more Eloy/Martins call.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andreas, I'm send this back round this week sorry so that the following things can be tidied up before this gets integrated. delete_template.php: The public suffix should be created as a string to which the template name is passed. $CFG->frontpage is a setting that controls what gets displayed on the front page. There are a couple of uses introduced by this patch that I don't think are right. Leads to: Debug info: ERROR: invalid input syntax for integer: "0,1" SELECT * FROM mdl_feedback_template WHERE course = $1 AND ispublic = $2 ORDER BY name [array ( 0 => '0,1', 1 => 1, )] Stack trace: • line 394 of /lib/dml/moodle_database.php: dml_read_exception thrown • line 232 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end() • line 678 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end() • line 1122 of /lib/dml/moodle_database.php: call to pgsql_native_moodle_database->get_records_sql() • line 1071 of /lib/dml/moodle_database.php: call to moodle_database->get_records_select() • line 1231 of /mod/feedback/lib.php: call to moodle_database->get_records() • line 81 of /mod/feedback/edit_form.php: call to feedback_get_template_list() • line 137 of /mod/feedback/edit.php: call to feedback_edit_use_template_form->set_form_elements() Are we sure we only want to allow people to create public templates from feedback instances on the frontpage? Given that we have the capability for it couldn't we just rely on that? When preparing the feedback edit form I think it would be better if we grouped options rather than prefixing the template name with an *. Alternatively if its decided to continue using the * we should probably have a string for it so that should it have a different meaning or understanding in other languages they can change it. Other than those this looks very good! Presently because of the above error I can't get too far into playing with it in the interface and I look forward to seeing that! In regards to which branches this will be applied to because it is a new feature (despite having all the hooks + capability there) it will only go into master. Perhaps if there is a strong enough case it can be backported to MOODLE_21_STABLE however that is more Eloy/Martins call. Cheers Sam
          Hide
          Andreas Grabs added a comment -

          Hi Sam,

          thank you very much for testing the code. I have used this var $CFG->frontpage stupidly . I should use SITEID for that. I was blind while using this. I will solve this shortly.
          I wanted to use the frontpage as a place for global templates because I could do an easier check for passing files in feedback_pluginfile(). The problem is the use of templates on other places including its files.
          The feature isn't really new. It was deactivated in 2.x doe to the new file-api. Now I could solve the sitewide access to files in public template and want it back.

          Best regards
          Andreas

          Show
          Andreas Grabs added a comment - Hi Sam, thank you very much for testing the code. I have used this var $CFG->frontpage stupidly . I should use SITEID for that. I was blind while using this. I will solve this shortly. I wanted to use the frontpage as a place for global templates because I could do an easier check for passing files in feedback_pluginfile(). The problem is the use of templates on other places including its files. The feature isn't really new. It was deactivated in 2.x doe to the new file-api. Now I could solve the sitewide access to files in public template and want it back. Best regards Andreas
          Hide
          Andreas Grabs added a comment -

          Hi Sam,

          now I replaced all use of $CFG->frontpage with SITEID. I think it should now work as expected.
          It would be really great if this changes go not only into master but at least into the stable 21. The feature was all the time there but only hidden, because problems with files.
          Thank your very much!

          Best regards
          Andreas

          Show
          Andreas Grabs added a comment - Hi Sam, now I replaced all use of $CFG->frontpage with SITEID. I think it should now work as expected. It would be really great if this changes go not only into master but at least into the stable 21. The feature was all the time there but only hidden, because problems with files. Thank your very much! Best regards Andreas
          Hide
          Sam Hemelryk added a comment -

          Hi Andreas,

          Thank you for cleaning up SITEID thing. I've been looking at this again this morning and have had a good chat with Eloy about this as well to get his feelings on a couple of things.

          First up the question of which branches this should be applied to - after talking to Martin yesterday and Eloy this morning it's been decided that this will only be integrated on master.

          As for the patch I am nearly entirely happy with it - there are several points I mentioned above that haven't been tidied up that should be however I am happy to create issues for that after the fact as they are all very minor.
          There is still one thing that catches my eye every time I look at these changes and that is that public templates can only be created within a front page feedback activity. I'm sorry but it doesn't feel right and it suggests larger problems with file contexts.
          Because of my feelings on this I asked Eloy to have a look at the changes and see what he thought and he identified the same thing - that limiting the creation of public templates to front page activities doesn't feel correct.
          I've had a look at the code and can see why it has been done like that; presently the template code assumes (nee requires) a course in regards to file handling and changing this functionality will require larger changes to feedbacks file and template handling.
          What we need to decide at the moment is whether or not we delay this and require those changes to be made before this gets integrated.
          If we integrate this now and then decide in the future to improve it with this functionality it is going to be a much harder task.

          Presently I am leaning towards delaying this issue so that we can improve this functionality to allow public templates to be created from within any feedback activity in which the user has `mod/feedback:createpublictemplate`.
          Using the capability alone feels much more correct to me than requiring a front page activity as well.
          Eloy had a great idea as well to change the context level for `mod/feedback:createpublictemplate` to CONTEXT_SYSTEM as well so that admin's can control the creation of public templates across the various levels within Moodle.
          Looking at the code I can see the following changes would need to be made:

          • Remove the SITEID checks when creating public templates.
          • Edit the capability changing contextlevel to the system context
          • Alter feedback_create_template so that if the template is to be public the course is set to either 0 or null - however null would require DB changes as feedback_template.course is presently not null.
          • Alter feedback_save_as_template, feedback_items_from_template, feedback_delete_template so that when storing and retrieving files things are done through a system context filearea.
          • Review other area's within feedback where a course is being passed so that to ensure they allow for templates without courses.

          Presently Andreas I've left this in integration so that we can get your thoughts on it and then decide what should happen here, if there is a strong enough argument for putting it in now it will go in.
          As mentioned above personally I think we should fix this up so that public templates can be created from any feedback module as this feels more complete than the frontpage only solution presently - also easier to fix up now than later.

          Cheers
          Sam

          Note: While looking at this issue I also spotted a bug within feedback whereby when deleting a template it doesn't delete the template items. I'll create a bug for this shortly.

          Show
          Sam Hemelryk added a comment - Hi Andreas, Thank you for cleaning up SITEID thing. I've been looking at this again this morning and have had a good chat with Eloy about this as well to get his feelings on a couple of things. First up the question of which branches this should be applied to - after talking to Martin yesterday and Eloy this morning it's been decided that this will only be integrated on master. As for the patch I am nearly entirely happy with it - there are several points I mentioned above that haven't been tidied up that should be however I am happy to create issues for that after the fact as they are all very minor. There is still one thing that catches my eye every time I look at these changes and that is that public templates can only be created within a front page feedback activity. I'm sorry but it doesn't feel right and it suggests larger problems with file contexts. Because of my feelings on this I asked Eloy to have a look at the changes and see what he thought and he identified the same thing - that limiting the creation of public templates to front page activities doesn't feel correct. I've had a look at the code and can see why it has been done like that; presently the template code assumes (nee requires) a course in regards to file handling and changing this functionality will require larger changes to feedbacks file and template handling. What we need to decide at the moment is whether or not we delay this and require those changes to be made before this gets integrated. If we integrate this now and then decide in the future to improve it with this functionality it is going to be a much harder task. Presently I am leaning towards delaying this issue so that we can improve this functionality to allow public templates to be created from within any feedback activity in which the user has `mod/feedback:createpublictemplate` . Using the capability alone feels much more correct to me than requiring a front page activity as well. Eloy had a great idea as well to change the context level for `mod/feedback:createpublictemplate` to CONTEXT_SYSTEM as well so that admin's can control the creation of public templates across the various levels within Moodle. Looking at the code I can see the following changes would need to be made: Remove the SITEID checks when creating public templates. Edit the capability changing contextlevel to the system context Alter feedback_create_template so that if the template is to be public the course is set to either 0 or null - however null would require DB changes as feedback_template.course is presently not null. Alter feedback_save_as_template, feedback_items_from_template, feedback_delete_template so that when storing and retrieving files things are done through a system context filearea. Review other area's within feedback where a course is being passed so that to ensure they allow for templates without courses. Presently Andreas I've left this in integration so that we can get your thoughts on it and then decide what should happen here, if there is a strong enough argument for putting it in now it will go in. As mentioned above personally I think we should fix this up so that public templates can be created from any feedback module as this feels more complete than the frontpage only solution presently - also easier to fix up now than later. Cheers Sam Note: While looking at this issue I also spotted a bug within feedback whereby when deleting a template it doesn't delete the template items. I'll create a bug for this shortly.
          Hide
          Andreas Grabs added a comment -

          Hi Sam,

          I understand the problem with public templates only located on the frontpage. I had some problems with fileaccess while using this template.
          I could do this as you explained. To delete templates I have to show all public templates in any feedback on any course if the user has the capability "mod/feedback:createpublictemplate". This capability must be given on the system-context, am I right?
          Nevertheless your suggestion sounds good. I will play around this and inform you again.
          Thank you very much for your detailed explanation.

          Andreas

          Show
          Andreas Grabs added a comment - Hi Sam, I understand the problem with public templates only located on the frontpage. I had some problems with fileaccess while using this template. I could do this as you explained. To delete templates I have to show all public templates in any feedback on any course if the user has the capability "mod/feedback:createpublictemplate". This capability must be given on the system-context, am I right? Nevertheless your suggestion sounds good. I will play around this and inform you again. Thank you very much for your detailed explanation. Andreas
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Sam, Andreas,

          thanks a lot for the "evolution" of the issue, the last proposal sounds really great and avoids the "must have feedback @ front page" thing.

          So I'm reopening this now... ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Sam, Andreas, thanks a lot for the "evolution" of the issue, the last proposal sounds really great and avoids the "must have feedback @ front page" thing. So I'm reopening this now... ciao
          Hide
          Andreas Grabs added a comment -

          Hi Sam & Eloy,

          I agree to all you said. Now I have rewritten all this things as sam suggested.
          Now all users who has the createpublictemplate capability on system context are able to create public templates in any course.
          The template listing for using these templates is now grouped by a selectgroups element.
          Also the delete_template page lists the templates grouped by "course" or "public". So there is a better structure.
          The branch MOODLE_20_STABLE I have left out. I think it is not so important for that.
          Thank you very much for your effort on testing my chaotic code .

          Best regards
          Andreas

          Show
          Andreas Grabs added a comment - Hi Sam & Eloy, I agree to all you said. Now I have rewritten all this things as sam suggested. Now all users who has the createpublictemplate capability on system context are able to create public templates in any course. The template listing for using these templates is now grouped by a selectgroups element. Also the delete_template page lists the templates grouped by "course" or "public". So there is a better structure. The branch MOODLE_20_STABLE I have left out. I think it is not so important for that. Thank you very much for your effort on testing my chaotic code . Best regards Andreas
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Side note: it looks better wow, thanks, although I think we should create some new issue to fix a lot of "coding-standards" the module is breaking (surely for master only).

          Things like spaces after //, or incorrect spacing in practically all if/else/for sentences should be fixed. Not a blocker at all, but better we try to follow the rules.

          Note this side note is separated 100% from this issue, issuing my +0.2 here and leaving it to Sam.

          Show
          Eloy Lafuente (stronk7) added a comment - Side note: it looks better wow, thanks, although I think we should create some new issue to fix a lot of "coding-standards" the module is breaking (surely for master only). Things like spaces after //, or incorrect spacing in practically all if/else/for sentences should be fixed. Not a blocker at all, but better we try to follow the rules. Note this side note is separated 100% from this issue, issuing my +0.2 here and leaving it to Sam.
          Hide
          Andreas Grabs added a comment -

          Hi Eloy,
          what is the best way to correct the coding-standards? Should I go from this state here or from the current master or MOODLE_21_STABLE?
          Can I do all in one branch and resolve more than one issue with that branch?
          It will be a big job and I do not want to leave some things.
          Best regards Andreas

          Show
          Andreas Grabs added a comment - Hi Eloy, what is the best way to correct the coding-standards? Should I go from this state here or from the current master or MOODLE_21_STABLE? Can I do all in one branch and resolve more than one issue with that branch? It will be a big job and I do not want to leave some things. Best regards Andreas
          Hide
          Sam Hemelryk added a comment -

          Hi Andreas,

          Thanks for making those changes - things look much better and this has been integrated now (Thanks for fixing the issue with deleting template items at the same time )
          I've also created MDL-29804 to clean up the coding style as suggested by Eloy - he is quite right it would be nice to tidy that up in master. The best point to tackle that by the way Andreas would be after this weekly release so that you avoid any/all conflicts by using the most up to date master.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andreas, Thanks for making those changes - things look much better and this has been integrated now (Thanks for fixing the issue with deleting template items at the same time ) I've also created MDL-29804 to clean up the coding style as suggested by Eloy - he is quite right it would be nice to tidy that up in master. The best point to tackle that by the way Andreas would be after this weekly release so that you avoid any/all conflicts by using the most up to date master. Cheers Sam
          Hide
          Andreas Grabs added a comment -

          Hi Sam,
          Thank you for creating this new issue. Am I right, the changes here will only get into the master for the version 2.2 so I only need to clean the code on the master-branch?
          Andreas

          Show
          Andreas Grabs added a comment - Hi Sam, Thank you for creating this new issue. Am I right, the changes here will only get into the master for the version 2.2 so I only need to clean the code on the master-branch? Andreas
          Hide
          Sam Hemelryk added a comment -

          Hi Andreas,
          You are spot on in that these changes have only been integrated for master, and that the coding style should only be tidied up in master.
          Unless there is a good reason we only make cosmetic changes such as coding style within master so that we don't increase the risk of breaking people custom code/modifications between minor releases on the stable branch.
          Let me know if you have any questions regarding the clean up and I'll be more than happy to help

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andreas, You are spot on in that these changes have only been integrated for master, and that the coding style should only be tidied up in master. Unless there is a good reason we only make cosmetic changes such as coding style within master so that we don't increase the risk of breaking people custom code/modifications between minor releases on the stable branch. Let me know if you have any questions regarding the clean up and I'll be more than happy to help Cheers Sam
          Hide
          Rossiani Wijaya added a comment -

          The patch works well in master.

          However, just wondering if some improvement is needed for labeling question-item. Currently if label field is empty, on preview page, the question is display as '() question_name'. I think it looks better if empty bracket is not display if label is not set.

          I'm passing this issue.

          Show
          Rossiani Wijaya added a comment - The patch works well in master. However, just wondering if some improvement is needed for labeling question-item. Currently if label field is empty, on preview page, the question is display as '() question_name'. I think it looks better if empty bracket is not display if label is not set. I'm passing this issue.
          Hide
          Andreas Grabs added a comment -

          Hm, I also thought about the label. I think a good way could be a default value like "no label" if empty. Shall I open a new issue for that?
          Best regards
          Andreas

          Show
          Andreas Grabs added a comment - Hm, I also thought about the label. I think a good way could be a default value like "no label" if empty. Shall I open a new issue for that? Best regards Andreas
          Hide
          Rossiani Wijaya added a comment -

          Thanks Andreas for the suggestion.

          I created MDL-29823 to improve the label.

          Show
          Rossiani Wijaya added a comment - Thanks Andreas for the suggestion. I created MDL-29823 to improve the label.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Many thanks for all the hard work. This is now part of Moodle, your favorite LMS.

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Many thanks for all the hard work. This is now part of Moodle, your favorite LMS. Closing as fixed, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: