Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pdfannotator comment subscription like forum fixes #20. #61

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lucaboesch
Copy link
Contributor

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.

@lucaboesch lucaboesch force-pushed the configuresubscription branch 6 times, most recently from 98d642c to f2a6133 Compare November 2, 2021 16:27
@lucaboesch lucaboesch force-pushed the configuresubscription branch 4 times, most recently from df6803a to d7de23f Compare November 30, 2021 10:47
@nisaDoc
Copy link

nisaDoc commented Oct 7, 2022

Hello,
thank you for your Pull Request. I think you should be finish with the implementation right?
Actually, we are working on another issues in PDFAnnotator. We will check your pull request later.

Best regards,
Nisa

@lucaboesch lucaboesch force-pushed the configuresubscription branch 10 times, most recently from 150346f to 381ad4c Compare November 29, 2022 11:22
@lucaboesch lucaboesch force-pushed the configuresubscription branch 7 times, most recently from c72bf4a to d12fb3f Compare May 24, 2023 07:22
@lucaboesch lucaboesch changed the base branch from master to main May 24, 2023 07:30
@lucaboesch lucaboesch force-pushed the configuresubscription branch 5 times, most recently from cc0f732 to 0777be9 Compare May 30, 2023 16:30
@lucaboesch lucaboesch force-pushed the configuresubscription branch 6 times, most recently from b9d9cee to 5faf44f Compare April 3, 2024 22:31
@lucaboesch lucaboesch force-pushed the configuresubscription branch 14 times, most recently from f2e6fe5 to 218b6b1 Compare June 13, 2024 05:58
@lucaboesch
Copy link
Contributor Author

Could this be re-taken up into consideration?

Copy link
Contributor

@t-schroeder t-schroeder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this pull request. I've made some comments. Can you have a look at them?

$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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it make sense to use the constant PDFANNOTATOR_CHOOSESUBSCRIBE here for the default value instead of '0'?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

See https://moodledev.io/docs/4.4/apis/commonfiles#libphp

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use the short array syntax, especially since this is a new function. See https://moodledev.io/general/development/policies/codingstyle#array-syntax

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this class used? I can't find any other PHP files referencing it.

@t-schroeder
Copy link
Contributor

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.

@lucaboesch lucaboesch force-pushed the configuresubscription branch from 218b6b1 to ce680e6 Compare December 6, 2024 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants