mailinglist: Updated to new standard #277
No reviewers
Labels
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
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
harmen/hypha!277
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "newtext_mailing"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
I reviewed the first half, up to including
addressesView(). More later.Is this a good idea? I can imagine this will cause subtle breakage if the classname is ever changed (since that will then influence the XML schema). Maybe keeping this explicit is better?
This should be named defaultViewRender I think?
Returning errors is not yet handled, right?
I suspect that at least
$emailneeds to be encoded here? IIRC we added anxpath_encode()function or something similar a while ago.Did you test this? IIRC findXPath by default only looks at children of the the elements you run it on (so if
$addresses->children()are theaddresselements, this will only look at their children). IIRC you can pass a custom axis tofindXpath, or just call on$addressesinstead of its children.I would
htmlspecialcharsthe$confirmUrlhere.this should also encode xpath
Should this use find rather than getOrCreate? With find, it will look recursively, which is not what is intended I think? Maybe this should use
get()instead?Should this find be limited in scope to the addresses element? Maybe alos use './' to only get direct children rather than look recursively? This also applies to other findXPaths in the code.
What is the idea behind this extra field? If we want to keep the e-mail address, why not just in the regular e-mail field?
Finished review, added more inline comments :-)
@ -409,2 +436,3 @@notify('success', ucfirst(__('ml-successfully-unsubscribed')));return ['redirect', $this->constructFullPath($this->pagename)];return ['redirect', $this->path()];I think this can return 404 to get a proper 404 HTTP response? Same for a few other places?
htmlspecialchars
This only adds the send button when in draft, is that intentional?
I would use
json_encode()for all these javascript strings.I think this can use
constructFullPath()?Why do we include the mailing id in the POST parameters at all? Only for this consistency check it seems? Why not leave it out of the form entirely? Originally, it seems it was used to distinguish between new mailings and edited mailings, but this is now done using arguments to helper functions, so just putting the id in the URL seems sufficient?
Huh? When status is SENT, this throes away the edit and says "successfully sent"? What's the idea here?
This should be
field-name-email.These should also be
field-name-xxx.@ -762,3 +784,1 @@private function testSendMailing($mailingId, $email) {if (!$this->hasSender()) {throw new \Exception(__('ml-no-sender'));/**This looks broken:
<field-name-labelsubjectFieldName]]">Also,
</field-name-label>is not valid HTML tag? What's going on here?Also,
[[subjectFieldName]]should be[[field-name-subject]]I think?@ -769,0 +813,4 @@];$form = $this->createMailingForm($formData);$cancelPath = $this->substituteSpecial(self::PATH_MAILS_VIEW_ID, ['id' => $mailingId]);This mixes up two checks:
!hasSender()now showslogin-to-edit(which is wrong), and there is no actual!isUser()check (seems it was not there before either).Shouldn't this use getSenderData?
This can be a normal
HTMLForm.htmlspecialchars on links
htmlspecialchars on links
updated it
true, i'll change this
changed it
changed it
It's tested and now tested again, so it does work. That said, I'll look into this.
Removing
->children()also works. :-)done
done
Updated this troughtout the class
If you keep it in the same field and the same email address would subscribe, that record would be re-used.
@ -409,2 +436,3 @@notify('success', ucfirst(__('ml-successfully-unsubscribed')));return ['redirect', $this->constructFullPath($this->pagename)];return ['redirect', $this->path()];updated
done
@ -409,2 +436,3 @@notify('success', ucfirst(__('ml-successfully-unsubscribed')));return ['redirect', $this->constructFullPath($this->pagename)];return ['redirect', $this->path()];done
Is that problematic? And if so, you could also change the subscription code to not reuse an UNSUBSCRIBED status record?
@ -409,2 +436,3 @@notify('success', ucfirst(__('ml-successfully-unsubscribed')));return ['redirect', $this->constructFullPath($this->pagename)];return ['redirect', $this->path()];Yes, it is. :-)
@ -409,2 +436,3 @@notify('success', ucfirst(__('ml-successfully-unsubscribed')));return ['redirect', $this->constructFullPath($this->pagename)];return ['redirect', $this->path()];It was already like this and take too much time to implement, so I consider this outside of the scope of this pull request.
@ -409,2 +436,3 @@notify('success', ucfirst(__('ml-successfully-unsubscribed')));return ['redirect', $this->constructFullPath($this->pagename)];return ['redirect', $this->path()];Right, I later noticed that sending a message removes it from draft, so that indeed makes sense :-)
@ -409,2 +436,3 @@notify('success', ucfirst(__('ml-successfully-unsubscribed')));return ['redirect', $this->constructFullPath($this->pagename)];return ['redirect', $this->path()];Why would this take time to implement? I meant this:
@ -409,2 +436,3 @@notify('success', ucfirst(__('ml-successfully-unsubscribed')));return ['redirect', $this->constructFullPath($this->pagename)];return ['redirect', $this->path()];done
@ -762,3 +784,1 @@private function testSendMailing($mailingId, $email) {if (!$this->hasSender()) {throw new \Exception(__('ml-no-sender'));/**fixed it
@ -409,2 +436,3 @@notify('success', ucfirst(__('ml-successfully-unsubscribed')));return ['redirect', $this->constructFullPath($this->pagename)];return ['redirect', $this->path()];I've removed it.
@ -409,2 +436,3 @@notify('success', ucfirst(__('ml-successfully-unsubscribed')));return ['redirect', $this->constructFullPath($this->pagename)];return ['redirect', $this->path()];... So it is fixed now.. by also replacing
"by'@ -409,2 +436,3 @@notify('success', ucfirst(__('ml-successfully-unsubscribed')));return ['redirect', $this->constructFullPath($this->pagename)];return ['redirect', $this->path()];fixed
true, but I would rather return in all the WymHTMLForm, it's almost no overhead and way more consistent.
@ -769,0 +813,4 @@];$form = $this->createMailingForm($formData);$cancelPath = $this->substituteSpecial(self::PATH_MAILS_VIEW_ID, ['id' => $mailingId]);I blame my laziness.
@ -769,0 +813,4 @@];$form = $this->createMailingForm($formData);$cancelPath = $this->substituteSpecial(self::PATH_MAILS_VIEW_ID, ['id' => $mailingId]);fixed it
@ -769,0 +813,4 @@];$form = $this->createMailingForm($formData);$cancelPath = $this->substituteSpecial(self::PATH_MAILS_VIEW_ID, ['id' => $mailingId]);thank you for catching this. :-)
@ -409,2 +436,3 @@notify('success', ucfirst(__('ml-successfully-unsubscribed')));return ['redirect', $this->constructFullPath($this->pagename)];return ['redirect', $this->path()];fixed it
@ -769,0 +813,4 @@];$form = $this->createMailingForm($formData);$cancelPath = $this->substituteSpecial(self::PATH_MAILS_VIEW_ID, ['id' => $mailingId]);fixed
done
done
@ -409,2 +436,3 @@notify('success', ucfirst(__('ml-successfully-unsubscribed')));return ['redirect', $this->constructFullPath($this->pagename)];return ['redirect', $this->path()];removed it
That also seems a bit convoluted. I would just keep it in the same space, to keep the structure simple and equal, and adapt other code to match. Most other code that deals with subscriptions already has to check for status anyway, so that is not particularly hard, right?
We could also consider preserving the current behaviour (removing email) in this PR, and move this change into a separate PR (a separate commit would be a good idea in any case, since it is quite a signficant behaviour change).
@ -409,2 +436,3 @@notify('success', ucfirst(__('ml-successfully-unsubscribed')));return ['redirect', $this->constructFullPath($this->pagename)];return ['redirect', $this->path()];Huh? Why is replacing " by ' needed? Javascript strings can be enclosed in " just fine, right? And
json_encodewill have taken care of any needed escaping already? If you replace the quote type, and your encoded strings contain quotes, I think you will actually break things?Because, IMHO, it adds complexity (e.g. the display code must now handle two fields for e-mail rather than one).
.... I've already retracted my previous comments. sorry. It just didn't click before.. Now it does. :-)
fixed
Yay! Also, deleting comments is sometimes annoying, since I still see them in e-mail notification but cannot use the link in the e-mail anymore to see the comments. Editing them to indicate they are no longr relevant might be better in such cases :-)
Looks good overall. I left a few comments inline around a common theme.
Also, I was missing the change to
unsubscribeAction()to handle aredirect, but I noticed this was already included in the previous commit (but still unused there). Not really a problem, though.@ -411,0 +448,4 @@if (!isAdmin()) {notify('error', __(isUser() ? 'admin-rights-needed-to-perform-action' : 'login-to-perform-action'));return ['redirect', $this->path()];}We've discussed this before, whether to show an error or success when the wanted state is already the current state, but in this case I really wonder if this is the right approach. Saying "reminder sent" explicitly indicates that something actually happened. In the case that something is wrong and e-mails are not received, it really does not help to have additional misunderstanding about whether something was even sent in the first place.
I realize that this is probably a corner case because the reminder button will not normally be shown when a reminder was already sent, but users will probably manage to keep open an old interface somewhere and then get confused about the message sent.
Should this authenticate using a code? Why not just check
isUser()? AFAIU there is never a need for a non-logged-in user to send reminders, so why use a code here? That only leads to additional exposure of the code, which can actually allow (semi-malicious) logged in users to craft a confirmation url, bypassing the usual consent-mechanism for subscriptions.Same here: Why use the code rather than logged in status?
Reading again I see that the code is also used to identify the e-mail address to remind, but then I would rather pass the e-mailaddress to confirm.
This also allows deleting confirmed addresses, which is not what the commit message says. Is that intentional?
@ -411,0 +535,4 @@$code = $address->getAttribute(self::FIELD_NAME_CONFIRM_CODE);$address->setAttribute(self::FIELD_NAME_REMINDED, true);$this->xml->saveAndUnlock();Again, I would use an
isAdmin()check inunsubscribeAction()rather than reusing the existing unsubscription code. Here, leaking the code is less of an issue, but checkingisAdmin()in the action makes it more explicit that unsubscription is also allowed by admins.Maybe this should be "already reminded" to emphasize that reminding can only be done once?
@ -411,0 +448,4 @@if (!isAdmin()) {notify('error', __(isUser() ? 'admin-rights-needed-to-perform-action' : 'login-to-perform-action'));return ['redirect', $this->path()];}This is indeed an edge case. The remind action can and should only be triggered one time. If the remind action is triggered a second time, there is no need to send another reminder, and also no need to inform the user that this edge case occurred, because for the user the end result is the same.
@ -411,0 +448,4 @@if (!isAdmin()) {notify('error', __(isUser() ? 'admin-rights-needed-to-perform-action' : 'login-to-perform-action'));return ['redirect', $this->path()];}fixed it
@ -411,0 +448,4 @@if (!isAdmin()) {notify('error', __(isUser() ? 'admin-rights-needed-to-perform-action' : 'login-to-perform-action'));return ['redirect', $this->path()];}fixed
@ -411,0 +448,4 @@if (!isAdmin()) {notify('error', __(isUser() ? 'admin-rights-needed-to-perform-action' : 'login-to-perform-action'));return ['redirect', $this->path()];}fixed
@ -411,0 +535,4 @@$code = $address->getAttribute(self::FIELD_NAME_CONFIRM_CODE);$address->setAttribute(self::FIELD_NAME_REMINDED, true);$this->xml->saveAndUnlock();fixed
@ -411,0 +448,4 @@if (!isAdmin()) {notify('error', __(isUser() ? 'admin-rights-needed-to-perform-action' : 'login-to-perform-action'));return ['redirect', $this->path()];}I've updated the code, but still don't think it's needed.
done
@ -411,0 +448,4 @@if (!isAdmin()) {notify('error', __(isUser() ? 'admin-rights-needed-to-perform-action' : 'login-to-perform-action'));return ['redirect', $this->path()];}I think this entirely depends on the timeframe. If two users click the remind button at the same time, then the result is pretty much the same, yes: A reminder has been sent. However, if more time passes, then the message might cause a user to (rightfully) assume that another reminder has been sent (and e.g. tell the reminded user that he should have received another e-mail, which is then untrue). And even when both presses occur in short succession, this might lead user to believe that two reminder emails have been sent, which is not actually the case.
@ -411,0 +448,4 @@if (!isAdmin()) {notify('error', __(isUser() ? 'admin-rights-needed-to-perform-action' : 'login-to-perform-action'));return ['redirect', $this->path()];}Would it not be better to reverse this check? E.g. "only pending addresses can be deleted"? Then this code remains working when additional statuses are added later.
@ -411,0 +535,4 @@$code = $address->getAttribute(self::FIELD_NAME_CONFIRM_CODE);$address->setAttribute(self::FIELD_NAME_REMINDED, true);$this->xml->saveAndUnlock();Really? I do not see any change in
unsubscribeActionand the url still contains the unsubscribe code?typo: ent -> sent
@ -411,0 +535,4 @@$code = $address->getAttribute(self::FIELD_NAME_CONFIRM_CODE);$address->setAttribute(self::FIELD_NAME_REMINDED, true);$this->xml->saveAndUnlock();The unsubscribeAction is used for both admins and subscribers. So the
isAdmin()isn't a good idea. I could make an extra action for the admins, but why, this one is already there?!@ -411,0 +535,4 @@$code = $address->getAttribute(self::FIELD_NAME_CONFIRM_CODE);$address->setAttribute(self::FIELD_NAME_REMINDED, true);$this->xml->saveAndUnlock();So, yeah, I left it as it is.
@ -411,0 +448,4 @@if (!isAdmin()) {notify('error', __(isUser() ? 'admin-rights-needed-to-perform-action' : 'login-to-perform-action'));return ['redirect', $this->path()];}true
@ -411,0 +535,4 @@$code = $address->getAttribute(self::FIELD_NAME_CONFIRM_CODE);$address->setAttribute(self::FIELD_NAME_REMINDED, true);$this->xml->saveAndUnlock();That's not really "fixed" :-)
I can imagine that the same action can be authenticated using
isAdmin(), or failing that, require a code to be specified? Or two actions that call the same function to do the actual work?As for why: it seems cleaner to me when the authentication code intended to be used for the recipient is not exposed elsewhere. Additionally, adding the isAdmin() check to
unsubscribeActionmakes it more clear to someone reading the code that unsubscription can be done by admins (iow, all access control appears in the same place, namely the action methods).However, as I said, leaking the unsubscribe code is not as problematic as leaking the confirm code, so when I fail to convince you, leaving this as is is also an option for me :-)
fixed
@ -411,0 +535,4 @@$code = $address->getAttribute(self::FIELD_NAME_CONFIRM_CODE);$address->setAttribute(self::FIELD_NAME_REMINDED, true);$this->xml->saveAndUnlock();implemented new method.
fixes #269