catalyst / moodle-MDL-70329

GNU General Public License v3.0
5 stars 4 forks source link

qbank_comment: Create plugin to add comments to questions #32

Open mattporritt opened 3 years ago

mattporritt commented 3 years ago

To support:

Create a qbank plugin called qbank_comment that allows question makers to add comments on questions.

High level requirements/ TODO:

mattporritt commented 3 years ago

@timhunt and @lucaboesch below is an initial thought I had. Can you share your thoughts, and clarify the requirement here if you can.

Should question comments follow the question or the question bank? Thinking about it it can go two ways:

  1. Comments are “global”/ attached to a question
  2. Comments are attached to a question instance in a question bank

For 1. this might be difficult with Moodles permission model. Questions don't exist in a context they are associated to a question bank. But I can see the usefulness of a global comment on a question that gives information about the question that you don't want students seeing. e.g. "3rd year students always have trouble with this topic area"

I think 2. is more likely in the spirit of the user story. But it might feel "weird" when a user has access to many questionabanks that share the same question and the comments on the question differs between banks

timhunt commented 3 years ago

Actually, I know previously we discussed questions being immutable, so they could be referenced by several different question bank items, but acutally that does not work (at least not simply).

The problem is the that questions have fields like question text, feedback etc. which have embedded files. Therefore, each question can only exist in one context. Therefroe, re-using them probably causes more pain than it solves.


Anyway, I was expecting the comments to be attached to the question bank item. I never considered anything else.

mattporritt commented 3 years ago

Based on discussion with the team we are going to go with approach 2. Comments are attached to a question instance in a question bank.

We should also explore the existing Moodle comment API for this: https://docs.moodle.org/dev/Comment_API

As an initial feature all existing comments will be copied when a question with comments is added/copied to a new questionbank. I'll make an issue for a feature to make this a ui option.

I'll also make an issue for a feature so users can subscribe to question comments so they are notified when a question in a question bank is commented on

abonaccorso commented 3 years ago

As also discussed in the meeting on 27th April, we have to clarify what happens to comments when exporting/importing or backing up/restoring questions from the same or another system. But this may be clarified later on.

mattporritt commented 3 years ago

I've created MDL-71642 for this issue. Please make a branch with the same name and make any pull requests against this branch

guillogo commented 3 years ago

Hi, I've added the capabilities in this PR: https://github.com/catalyst/moodle-MDL-70329/pull/128 however were created as the other old capabilities have been created before for moodle/question:xxxx but these can be changed if required. Thank you.

mattporritt commented 3 years ago

@guillogo PR #128 approved and merged

safatshahin commented 3 years ago

https://user-images.githubusercontent.com/55496911/122230656-3075b400-cefd-11eb-8eb8-fa7562e7c136.mov

Hi @abonaccorso @timhunt @lucaboesch I have implemented the comments as a prototype in the previewquesiton plugin, please let me know your feedback and if you think it should be done as a different plugin, it can be done as well. Just one important note: I think the Close preview need to change and we should use redirect the page instead of a pop up if we are using the comments in the preview page as the base page needs reloading to show the correct number of comments,

Thanks

abonaccorso commented 3 years ago

@safatshahin I had a look at the comments in qbank_previewquestion. I like

I added some user stories on page 10 here: https://docs.google.com/document/d/1JPBTqpCbtdRFTJuX-GIaxKyxtFLoUdPO/edit#. They are not yet complete and I am not sure if everyone agrees. You may find further information on WHERE the question may be used in the same document

And further information on permissions regarding comments in the above mentioned document

Here is the most important things I thought

  1. students will NEVER see those comments
  2. teachers should see that number of questions in every question list they encounter (I tried to think of all in the document specified above, but am not sure I found all)
  3. teachers should see comments not only in the question preview, but also when creating/editing questions and when grading questions and when.
  4. teachers should in some way be able to decide it they get notified on comments or not. I am not sure if this should be a decision by the user or a general setting per question bank. We should discuss this.
  5. I am not sure about the place of the comments.
guillogo commented 3 years ago

Hi @safatshahin

https://github.com/catalyst/moodle-MDL-70329/pull/203 has been reviewed.

Thank you. Guillermo.

guillogo commented 3 years ago

Hi @safatshahin the PR has been merged :) Thank you.

abonaccorso commented 3 years ago

Just had a look at comments and found the following things that I think we should improve:

  1. The title for comments is "Collaps all". It should say "Comments"
  2. The title for comments is smaller than the other collapsible titles - it should be the same size (so should maybe technical Informations as it is also collapsible)
  3. When making the preview window smaller or bigger, the comments window does not adapt well (see screenshot). I think it should always be right aligned with the box, in which the question text is visible (see screenshot below)
  4. Instead of saying "save comment" I would just write "save". Maybe we want to have this as a button? To me this seems more like Moodle. (It would even be nicer if my comments was saved when I hit "enter" (but I guess this is something that does not work in Moodle anyway)
  5. The link for "Download this question in xml format" and "Technical information" are too crowded. There should be more space, I suppose.
  6. From my point of view it is not necessary to highlight new comments in yellow.

image

  1. The cronology of comments is only persistent as long as they do not exceed the max. amount of comments/lines where a new page is introduced. Once a second page is introduced, the chronology of comments is broken. Check: https://quiz4.catalyst-au.net/question/bank/previewquestion/preview.php?id=34&cmid=167 (comments on the second page are older than the last comments on the first page)
  2. I am no longer sure if we should have comments besides the other collapsible title (even though at first this was my idea). This is inconsistent with Moodle layout elsewhere and it causes strange line breaks for titles of different functions when the preview window is rather small

image

I would suggest to place comments directly below the line of buttons for the question preview and collapse all titles, including comments when opening the preview. One can then decide what to open and what to keep collapsed.

I like the fact, that comments can be deleted - also the ones made by others. And I also like the fact, that overlong comments without spaces are "restricted" in a box with scrollbar.

Having comments in the preview of a question is a good idea, I think - the more I think about it, the better I like it

abonaccorso commented 3 years ago

Irregardless of comments, there is a small issue with the buttons for actions on the preview: spaces are missing. It would be nice if we could add them

image

Luca created a tracker issue (MDL-72216) for this and will fix it soon.

lucaboesch commented 3 years ago

I created an issue and submitted a patch for this (the button spaces) in Moodle tracker https://tracker.moodle.org/browse/MDL-72216 but it may affect only the now supported versions and maybe has to be ported here.

abonaccorso commented 3 years ago

Regarding my points 1. and 2. from above I realized that I misunderstood something (thanks @lucaboesch): "Collapse all" is the usual "Collapse all" hyperlink that is meant to collapse (or expand) all existing titles (as "Attempt options" and "Display options"), not the title for comments. So I think

abonaccorso commented 3 years ago

Luca and I talked it through this morning and will present a wireframe tomorrow in our Catch up.

abonaccorso commented 3 years ago

Regarding my point 4 - I reported this here https://tracker.moodle.org/browse/MDL-71642.

abonaccorso commented 3 years ago

If I see comments in the question preview depends on where I opened the preview: If I open the preview of an already existing question with already existing comments, I CANNOT see comments if I open the preview via "edit question page".

How to reproduce

  1. Create a quiz
  2. Create a question
  3. open the preview of the question
  4. add some comments
  5. go back to the quiz and click on the cogwheel to edit the questions
  6. click on "Preview"
  7. you won't see the comments added in 4.
mattporritt commented 3 years ago

HI @safatshahin , can you please follow up with @abonaccorso comment. Also @abonaccorso I can't see your comment in MDL-71642, is it just me?

abonaccorso commented 3 years ago

@mattporritt no it's no only you. I posted the wrong link. I reported it here (along with other things we declared out of scope for this project): https://tracker.moodle.org/browse/MDL-71935?focusedCommentId=866860&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-866860 (and refered to MDL-71642). Sorry for the confusion!

safatshahin commented 3 years ago

Hi @mattporritt @abonaccorso My sincere apologies, I should have addressed this long ago but I think lost the notification with other ones. I believe this issue has been addressed but not yet pushed to the test site. I will do some cleanup and push everything on the test site. @abonaccorso will that be ok if I do a little data cleanup on the test site? like deleting the current courses and creating new ones? It won't affect the users. Thanks

abonaccorso commented 3 years ago

@safatshahin sure, do the cleanup if needed. Please let me know once you pushed the fixes to the test site, so I can pass it on to my colleagues to have a look at.