allow the creation of a "Public" feedback

Details

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!

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!

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

Andreas Grabs
added a comment - 08/Oct/11 10:22 PM 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

Paul Vaughan
added a comment - 09/Oct/11 4:30 AM 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.

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.

Sam Hemelryk
added a comment - 10/Oct/11 11:02 AM 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

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.

Andreas Grabs
added a comment - 10/Oct/11 2:15 PM 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

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!

Andreas Grabs
added a comment - 11/Oct/11 3:49 AM 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

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.

Sam Hemelryk
added a comment - 11/Oct/11 8:56 AM 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.

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 Grabs
added a comment - 11/Oct/11 3:44 PM 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

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 .

Andreas Grabs
added a comment - 12/Oct/11 5:07 AM 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

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.

Eloy Lafuente (stronk7)
added a comment - 13/Oct/11 6:20 AM 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

Eloy Lafuente (stronk7)
added a comment - 17/Oct/11 12:48 AM 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.

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

Andreas Grabs
added a comment - 17/Oct/11 2:23 AM 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

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.

Sam Hemelryk
added a comment - 17/Oct/11 6:23 AM 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

Andreas Grabs
added a comment - 17/Oct/11 12:01 PM 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

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

Sam Hemelryk
added a comment - 17/Oct/11 2:00 PM 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

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.

Rossiani Wijaya
added a comment - 18/Oct/11 6:14 PM 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.

Andreas Grabs
added a comment - 18/Oct/11 11:40 PM 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