harmen/hypha
1
0
Fork 0
Code Issues 142 Pull requests 4 Projects Releases Packages Wiki Activity

Peer reviewed article subscribe to discussion updates #281

Merged
laurensmartina merged 3 commits from peer-reviewed-article-subscribe-to-discussion-updates into master 2020-03-22 20:17:31 +00:00
Conversation 17 Commits 3 Files changed 3 +386 -199
laurensmartina commented 2019-12-29 14:54:14 +00:00 (Migrated from github.com)
Copy link
No description provided.
tammoterhark commented 2020-01-06 13:16:12 +00:00 (Migrated from github.com)
Copy link

image

Moet zijn: "Houd me op de hoogte."

![image](https://user-images.githubusercontent.com/6483317/71820300-01fc7400-308f-11ea-9322-88d7d145fd44.png) Moet zijn: "Houd me op de hoogte."
matthijskooijman (Migrated from github.com) requested changes 2020-01-20 12:55:40 +00:00
matthijskooijman (Migrated from github.com) left a comment
Copy link

I left some inline comments.

Overall, I wonder: Are you subscribing to an article as a whole? Or a single discussion? Should this be a choice to make?

I left some inline comments. Overall, I wonder: Are you subscribing to an article as a whole? Or a single discussion? Should this be a choice to make?
system/datatypes/peer_reviewed_article.php
@ -522,3 +530,3 @@
$dataFieldSuffix = '/new_' . $type;
$dataFieldSuffix = $this->constructDiscussionFieldSuffix($type);
matthijskooijman (Migrated from github.com) commented 2020-01-04 21:22:15 +00:00
Copy link

I do not really like using a ternary if in this way (where it does not produce a meaningful value but is only used for the side effects), for clarity I would prefer a normal if here (and below). Mostly preference, though.

I do not really like using a ternary if in this way (where it does not produce a meaningful value but is only used for the side effects), for clarity I would prefer a normal if here (and below). Mostly preference, though.
system/datatypes/peer_reviewed_article.php
@ -745,2 +733,2 @@
notify('success', ucfirst(__('art-comment-posted')));
return ['redirect', $this->constructFullPath($this->pagename)];
$confirmedComment = false;
$confirmedSubscriber = false;
matthijskooijman (Migrated from github.com) commented 2020-01-20 12:35:48 +00:00
Copy link

I think pending is no longer a good variable name here. Something like $confirmed is better, since it indicates that something was confirmed (even though that does mean it used to be pending). Also, I think that simply splitting this into two variables (e.g. $confirmedComment and $confirmedSubscription) is more clear.

Finally: Is there ever a case where you can confirm a subscription without confirming a comment? Probably not in practice, but maybe it is enough to track this separately just for abnormal circumstances?

I think pending is no longer a good variable name here. Something like `$confirmed` is better, since it indicates that something was confirmed (even though that does mean it used to be pending). Also, I think that simply splitting this into two variables (e.g. `$confirmedComment` and `$confirmedSubscription`) is more clear. Finally: Is there ever a case where you can confirm a subscription without confirming a comment? Probably not in practice, but maybe it is enough to track this separately just for abnormal circumstances?
system/datatypes/peer_reviewed_article.php
@ -1133,2 +1239,4 @@
$vars['email'] = __('art-email');
$vars['field-name-email'] = self::FIELD_NAME_DISCUSSION_COMMENTER_EMAIL . $dataFieldSuffix;
$vars['subscribe'] = __('art-subscribe');
$vars['info-button-subscribe'] = makeInfoButton('help-art-subscribe');
matthijskooijman (Migrated from github.com) commented 2020-01-20 12:49:01 +00:00
Copy link

Why so much code over the checkbox value here? Just any non-empty value would be sufficient, and then in the handling just check to see if the value is set?

There is also some code to pass a value for the checkbox as part of $formData, which just sets the same value again, which also seems pointless to me? Why is all this needed?

Why so much code over the checkbox value here? Just any non-empty value would be sufficient, and then in the handling just check to see if the value is set? There is also some code to pass a value for the checkbox as part of `$formData`, which just sets the same value again, which also seems pointless to me? Why is all this needed?
system/datatypes/peer_reviewed_article.php
@ -1241,2 +1359,4 @@
]);
$messageArgs = [
'name' => htmlspecialchars($name),
'link' => htmlspecialchars($linkToPage),
matthijskooijman (Migrated from github.com) commented 2020-01-20 12:46:30 +00:00
Copy link

I think this should not do htmlspecialchars, if anything it should do rawurlencode (since you're building a URL here. The htmlspecialchars should be done below, on the full $linkToUnsubscribe when it is inserted into the href attribute.

I think this should not do `htmlspecialchars`, if anything it should do `rawurlencode` (since you're building a URL here. The `htmlspecialchars` should be done below, on the full `$linkToUnsubscribe` when it is inserted into the `href` attribute.
matthijskooijman (Migrated from github.com) commented 2020-01-20 12:47:21 +00:00
Copy link

I wonder if this unsubsribe stuff should be appended, or maybe included in the __('art-comment-subscriber-body') string?

I wonder if this unsubsribe stuff should be appended, or maybe included in the `__('art-comment-subscriber-body')` string?
system/datatypes/peer_reviewed_article.php
matthijskooijman (Migrated from github.com) commented 2020-01-20 12:50:19 +00:00
Copy link

I haven't checked, but this looks like the subscribe checkbox is always shown, even for logged in users that are automatically subscribed currently (and do not need to enter or confirm their e-mail). Or am I mistaken?

I haven't checked, but this looks like the subscribe checkbox is *always* shown, even for logged in users that are automatically subscribed currently (and do not need to enter or confirm their e-mail). Or am I mistaken?
system/languages/en.php
matthijskooijman (Migrated from github.com) commented 2020-01-20 12:53:16 +00:00
Copy link

Should this be "Keep me informed..."?

Should this be "Keep *me* informed..."?
matthijskooijman (Migrated from github.com) commented 2020-01-20 12:54:40 +00:00
Copy link

Or maybe "Stay informed..."?

Or maybe "Stay informed..."?
system/languages/nl.php
@ -339,9 +339,11 @@
"art-block-resolved-subject" => "Breekpunt opgelost bij '[[title]]'",
matthijskooijman (Migrated from github.com) commented 2020-01-20 12:53:56 +00:00
Copy link

This seems like an unrelated change?

This seems like an unrelated change?
laurensmartina commented 2020-03-02 14:02:14 +00:00 (Migrated from github.com)
Copy link

Moet zijn: "Houd me op de hoogte."

fixed

> Moet zijn: "Houd me op de hoogte." fixed
laurensmartina (Migrated from github.com) reviewed 2020-03-02 14:14:20 +00:00
system/datatypes/peer_reviewed_article.php
@ -522,3 +530,3 @@
$dataFieldSuffix = '/new_' . $type;
$dataFieldSuffix = $this->constructDiscussionFieldSuffix($type);
laurensmartina (Migrated from github.com) commented 2020-03-02 14:14:19 +00:00
Copy link

fixed

fixed
laurensmartina (Migrated from github.com) reviewed 2020-03-02 14:42:31 +00:00
system/datatypes/peer_reviewed_article.php
@ -745,2 +733,2 @@
notify('success', ucfirst(__('art-comment-posted')));
return ['redirect', $this->constructFullPath($this->pagename)];
$confirmedComment = false;
$confirmedSubscriber = false;
laurensmartina (Migrated from github.com) commented 2020-03-02 14:42:31 +00:00
Copy link

fixed

fixed
laurensmartina (Migrated from github.com) reviewed 2020-03-02 15:04:39 +00:00
system/datatypes/peer_reviewed_article.php
@ -1241,2 +1359,4 @@
]);
$messageArgs = [
'name' => htmlspecialchars($name),
'link' => htmlspecialchars($linkToPage),
laurensmartina (Migrated from github.com) commented 2020-03-02 15:04:39 +00:00
Copy link

Don't want to risk it being excluded.

Don't want to risk it being excluded.
laurensmartina (Migrated from github.com) reviewed 2020-03-02 15:04:54 +00:00
system/datatypes/peer_reviewed_article.php
@ -1241,2 +1359,4 @@
]);
$messageArgs = [
'name' => htmlspecialchars($name),
'link' => htmlspecialchars($linkToPage),
laurensmartina (Migrated from github.com) commented 2020-03-02 15:04:53 +00:00
Copy link

fixed

fixed
laurensmartina (Migrated from github.com) reviewed 2020-03-02 15:27:16 +00:00
system/datatypes/peer_reviewed_article.php
@ -1133,2 +1239,4 @@
$vars['email'] = __('art-email');
$vars['field-name-email'] = self::FIELD_NAME_DISCUSSION_COMMENTER_EMAIL . $dataFieldSuffix;
$vars['subscribe'] = __('art-subscribe');
$vars['info-button-subscribe'] = makeInfoButton('help-art-subscribe');
laurensmartina (Migrated from github.com) commented 2020-03-02 15:27:16 +00:00
Copy link

Good to know. And a good indication that the workings of the HTMLForm is obscure.
Removed redundant code.

Good to know. And a good indication that the workings of the HTMLForm is obscure. Removed redundant code.
laurensmartina (Migrated from github.com) reviewed 2020-03-02 15:29:31 +00:00
system/datatypes/peer_reviewed_article.php
laurensmartina (Migrated from github.com) commented 2020-03-02 15:29:31 +00:00
Copy link

This part of the code is only used for non-users.

This part of the code is only used for non-users.
laurensmartina (Migrated from github.com) reviewed 2020-03-02 15:30:44 +00:00
system/languages/en.php
laurensmartina (Migrated from github.com) commented 2020-03-02 15:30:44 +00:00
Copy link

fixed

fixed
laurensmartina (Migrated from github.com) reviewed 2020-03-02 15:37:34 +00:00
system/languages/nl.php
@ -339,9 +339,11 @@
"art-block-resolved-subject" => "Breekpunt opgelost bij '[[title]]'",
laurensmartina (Migrated from github.com) commented 2020-03-02 15:37:33 +00:00
Copy link

moved from this commit

moved from this commit
matthijskooijman (Migrated from github.com) approved these changes 2020-03-16 16:18:14 +00:00
matthijskooijman (Migrated from github.com) left a comment
Copy link

Looks almost good to me. I left a few inline comments, which I also fixed in a fixup commit which I'll push in a minute.

I also added one new commit to prevent subscribers from being notified of their own comments (which seems unnecessary and maybe even confusing).

@laurensmartina, if you don't find anything wrong with these additions, I think the fixup commit can be squashed and the PR can be merged.

I did have two more usability questions (maybe for @tammoterhark or @giplt), which should not block merging this I think:

  • Is it sufficiently clear that people subscribe to just the discussion they commented in, and not all comments on the article?
  • The layout of the comment form is a bit messy in the default CSS now. Since this applies to the entire form, not just the new checkbox, this is probably out of scope for this PR, but might be good to improve at some point.
Looks *almost* good to me. I left a few inline comments, which I also fixed in a fixup commit which I'll push in a minute. I also added one new commit to prevent subscribers from being notified of their own comments (which seems unnecessary and maybe even confusing). @laurensmartina, if you don't find anything wrong with these additions, I think the fixup commit can be squashed and the PR can be merged. I did have two more usability questions (maybe for @tammoterhark or @giplt), which should not block merging this I think: - Is it sufficiently clear that people subscribe to just the discussion they commented in, and not all comments on the article? - The layout of the comment form is a bit messy in the default CSS now. Since this applies to the entire form, not just the new checkbox, this is probably out of scope for this PR, but might be good to improve at some point.
system/datatypes/peer_reviewed_article.php
@ -737,3 +726,4 @@
$comment = $commentNodeList->first();
if (!$comment instanceof HyphaDomElement) {
$this->xml->unlock();
notify('error', __('art-invalid-code'));
matthijskooijman (Migrated from github.com) commented 2020-03-16 14:11:08 +00:00
Copy link

This does not actually contain the container, it's a list of matched subcribers. I would just take the first one of that directly, e.g.:

		$subscriber = $this->xml->findXPath($xpath)->first();

Which returns null when not subscribers were matched.

This does not actually contain the container, it's a list of matched subcribers. I would just take the first one of that directly, e.g.: ```suggestion $subscriber = $this->xml->findXPath($xpath)->first(); ``` Which returns `null` when not subscribers were matched.
system/datatypes/peer_reviewed_article.php
@ -848,4 +905,4 @@
if (!$review) {
if (isUser()) {
$userEmail = $this->O_O->getUser()->getAttribute('email');
$userName = $this->O_O->getUser()->getAttribute('fullname');
matthijskooijman (Migrated from github.com) commented 2020-03-16 14:53:56 +00:00
Copy link

This check for equal to 'on' seems overly specific to me, especially since it is never explicitly given anywhere. The value passed in the POST request is normally equal to the HTML input checkbox element's "value" attribute, which is omitted here. It seems HTML5 explicitly specifies this to default to "on" when omitted, but I'm not sure it is nice to rely on that here (also HTML4 does not specify any default but requires the "value" attribute, so who knows what browsers might be sending?).

I would suggest simply relying on HTML-for to do the right thing and provide true-y or falsy value directly from dataFor, which matches the true that is entered as the default value. In practice, this works since when the checkbox is not checked, it is not part of the POST request at all, so the dataFor call will return null, which is falsey.

				if ($form->dataFor(self::FIELD_NAME_DISCUSSION_COMMENTER_SUBSCRIBE . $dataFieldSuffix)) {
This check for equal to 'on' seems overly specific to me, especially since it is never explicitly given anywhere. The value passed in the POST request is normally equal to the HTML input checkbox element's "value" attribute, which is omitted here. It seems HTML5 explicitly specifies this to default to "on" when omitted, but I'm not sure it is nice to rely on that here (also HTML4 does not specify any default but requires the "value" attribute, so who knows what browsers might be sending?). I would suggest simply relying on HTML-for to do the right thing and provide true-y or falsy value directly from `dataFor`, which matches the `true` that is entered as the default value. In practice, this works since when the checkbox is not checked, it is not part of the POST request at all, so the `dataFor` call will return `null`, which is falsey. ```suggestion if ($form->dataFor(self::FIELD_NAME_DISCUSSION_COMMENTER_SUBSCRIBE . $dataFieldSuffix)) { ```
matthijskooijman (Migrated from github.com) commented 2020-03-16 15:33:29 +00:00
Copy link

This also returns subscribers with status "unsubscribed", which are not changed to pending, so you cannot resubscribe after unsubscribing. I think the best fix would be to exclude unsubscribed entries in this xpath, so a new, pending, subscriber element will be created again.

This also returns subscribers with status "unsubscribed", which are not changed to pending, so you cannot resubscribe after unsubscribing. I think the best fix would be to exclude unsubscribed entries in this xpath, so a new, pending, subscriber element will be created again.
system/datatypes/peer_reviewed_article.php
@ -1235,3 +1355,3 @@
$subject = __('art-comment-subject', array(
$subject = __('art-comment-subject', [
'name' => $name,
matthijskooijman (Migrated from github.com) commented 2020-03-16 15:55:16 +00:00
Copy link

Probably better to use xpath_encode rather than manually quoting values here.

Probably better to use `xpath_encode` rather than manually quoting values here.
matthijskooijman commented 2020-03-16 16:28:23 +00:00 (Migrated from github.com)
Copy link

I've also rebased on top of master.

I've also rebased on top of master.
laurensmartina commented 2020-03-22 08:43:01 +00:00 (Migrated from github.com)
Copy link

@matthijskooijman I live the changes you made, and rebased the fixup

@matthijskooijman I live the changes you made, and rebased the fixup
tammoterhark commented 2020-03-22 08:57:09 +00:00 (Migrated from github.com)
Copy link

I think we will have to find out if the functionality is sufficiently clear. We'll see.

I think we will have to find out if the functionality is sufficiently clear. We'll see.
matthijskooijman commented 2020-03-22 09:47:49 +00:00 (Migrated from github.com)
Copy link

@laurensmartina what does "I live the changes you made" mean? Did you mean "reviewed"?

Regardless, shall we merge this, put it in test.destadsbron.nl and include it in the upcoming deployment?

@laurensmartina what does "I live the changes you made" mean? Did you mean "reviewed"? Regardless, shall we merge this, put it in test.destadsbron.nl and include it in the upcoming deployment?
laurensmartina commented 2020-03-22 20:16:48 +00:00 (Migrated from github.com)
Copy link

@matthijskooijman , it was a typo.. live should have been like.

@matthijskooijman , it was a typo.. `live` should have been `like`.
Sign in to join this conversation.
Reviewers
No reviewers
matthijskooijman
Labels
Clear labels   
Component: User interface

   
Component: Wymeditor

   
Help wanted

   
Level: Difficult

Structural changes, need discussion

  
Level: Easy

Under 4 hours of work

  
Level: Moderate

Under 16 hours of work

  
Pagetype: Festival

   
Pagetype: Mailinglist

   
Pagetype: Peer reviewed article

   
Pagetype: Text

   
Privacy GDPR AVG

   
status: has conflicts

   
Status: Needs changes

   
Status: Needs discussion

   
Status: Needs review

   
Status: Ready to merge

   
Status: Waiting for response

   
Type: Bug

   
Type: Enhancement

   
Type: Question

   
Usecase: De Stadsbron

   
Usecase: Koppelting

   
Usecase: MeetjeStad

   
Value: Coders

Enhancement of codebase, API, documentation

  
Value: Security

   
Value: Users

Improve user experience, add new features

  
Value: Visitors

Improve navigation, styling

No labels
Component: User interface
Component: Wymeditor
Help wanted
Level: Difficult
Level: Easy
Level: Moderate
Pagetype: Festival
Pagetype: Mailinglist
Pagetype: Peer reviewed article
Pagetype: Text
Privacy GDPR AVG
status: has conflicts
Status: Needs changes
Status: Needs discussion
Status: Needs review
Status: Ready to merge
Status: Waiting for response
Type: Bug
Type: Enhancement
Type: Question
Usecase: De Stadsbron
Usecase: Koppelting
Usecase: MeetjeStad
Value: Coders
Value: Security
Value: Users
Value: Visitors
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
harmen/hypha!281
No description provided.