Pdfannotator comment subscription like forum fixes #20. #61
Conversation
lucaboesch
force-pushed
the
configuresubscription
branch
6 times, most recently
from
98d642c
to
f2a6133
Compare
lucaboesch
force-pushed
the
configuresubscription
branch
4 times, most recently
from
df6803a
to
d7de23f
Compare
|
Hello, Best regards, |
lucaboesch
force-pushed
the
configuresubscription
branch
10 times, most recently
from
150346f
to
381ad4c
Compare
lucaboesch
force-pushed
the
configuresubscription
branch
7 times, most recently
from
c72bf4a
to
d12fb3f
Compare
lucaboesch
force-pushed
the
configuresubscription
branch
6 times, most recently
from
cc0f732
to
0777be9
Compare
lucaboesch
force-pushed
the
configuresubscription
branch
6 times, most recently
from
b9d9cee
to
5faf44f
Compare
lucaboesch
force-pushed
the
configuresubscription
branch
13 times, most recently
from
2316ae3
to
f2e6fe5
Compare
lucaboesch
force-pushed
the
configuresubscription
branch
from
f2e6fe5
to
218b6b1
Compare
|
Could this be re-taken up into consideration? |
| $string['subscriptionmode'] = 'Subscription mode'; | ||
| $string['subscriptionmode_help'] = 'When a participant is subscribed to a question it means they will receive notifications for questions. There are 4 subscription | ||
| mode options: | ||
| * Auto subscription - Everyone is subscribed initially to notifications for questions but can choose to unsubscribe at any time |
There was a problem hiding this comment.
You need to add an empty line before the list. Otherwise it will not be rendered correctly.
| $string['subscriptiondisabled'] = 'Subscription disabled'; | ||
| $string['subscriptionforced'] = 'Forced subscription'; | ||
| $string['subscriptionmode'] = 'Subscription mode'; | ||
| $string['subscriptionmode_help'] = 'When a participant is subscribed to a question it means they will receive notifications for questions. There are 4 subscription |
There was a problem hiding this comment.
I suggest changing the wording to: "will receive notifications for comments". Only the initial comment that you make when creating an annotation is usually called the question.
|
|
||
| // Define field forcesubscribe to be added to pdfannotator. | ||
| $table = new xmldb_table('pdfannotator'); | ||
| $field = new xmldb_field('forcesubscribe', XMLDB_TYPE_INTEGER, '1', null, XMLDB_NOTNULL, null, '0', 'useprotectedcomments'); |
There was a problem hiding this comment.
Wouldn't it make sense to use the constant PDFANNOTATOR_CHOOSESUBSCRIBE here for the default value instead of '0'?
There was a problem hiding this comment.
Nevermind. I forgot this file gets created by the XMLDB editor.
| $options[PDFANNOTATOR_FORCESUBSCRIBE] = get_string('subscriptionforced', 'pdfannotator'); | ||
| $options[PDFANNOTATOR_DISALLOWSUBSCRIBE] = get_string('subscriptiondisabled', 'pdfannotator'); | ||
| return $options; | ||
| } |
There was a problem hiding this comment.
What is the reason for putting this function in lib.php? This file should normally only contain callback functions or stuff for interfacing with Moodle Core. It seems like it would fit better into locallib.php.
There was a problem hiding this comment.
I just realized it doesn't really make a difference since locallib.php is being included in lib.php.
| @@ -90,7 +90,7 @@ function pdfannotator_display_embed($pdfannotator, $cm, $course, $file, $page = | |||
| $capabilities->useprintcomments = has_capability('mod/pdfannotator:printcomments', $context); | |||
| // 3. Comment editor setting. | |||
| $editorsettings = new stdClass(); | |||
| $editorsettings->active_editor = get_class(editors_get_preferred_editor(FORMAT_HTML)); | |||
| $editorsettings->active_editor = explode(',', get_config('core', 'texteditors'))[0]; | |||
There was a problem hiding this comment.
Is this change intentional? Probably not because this line has changed upstream since the pull request was created.
| function pdfannotator_get_subscriptionmode($id) { | ||
| global $DB; | ||
| $subscriptionmode = $DB->get_field('pdfannotator', 'forcesubscribe', array('id' => $id), $strictness = MUST_EXIST); |
There was a problem hiding this comment.
I would use the short array syntax, especially since this is a new function. See https://moodledev.io/general/development/policies/codingstyle#array-syntax
There was a problem hiding this comment.
Where is this class used? I can't find any other PHP files referencing it.
|
If I understand it correctly this pull request only allows managers to change the subscription behavior for individual questions. If someone creates a question that person is automatically subscribed to it. So they receive notifications for each new comment made to it. In what scenario would people who ask questions not want to be informed about answers to them? So I don't think that is what #20 was talking about at all. That was rather about being able to change the subscription of participants to individual PDFannotator instances, not to discussions they themselves created. The forum works that way: The subscription mode specifies how/if participants are subscribed to the particular forum instance and then you can separately subscribe to individual discussions. Would you consider changing your implementation to work this way? Otherwise I don't really see the benefit. |
The subscription setting will distinguish between optional
subscription, auto subscription, forced subscription and
subscription disabled for comments, auto subscription being the
default as it has been up to now.
If disabled or forced, no subscribe/unsubscribe menu entry is
shown.
Contrary to forum if you change this in hindsight, for
example it will not subscribe or unsubscribe all person
who have subscribed/unsubscribed to a comment.
Also, Behat tests are introduced hereby.