Subscription Refactoring #125

Merged
merged 26 commits into from Jan 26, 2013

4 participants

@splitbrain
Owner

This refactors the whole email subscription handling into a class and adds a few test cases.

This probably fixes FS#2580 but I'm not sure we should merge this before the release as this might still have some bugs.

@adrianlang it would be great if you could have a look at this and check that I did not do something stupid somewhere

I also have a few open questions:

  • theoretically all these functions could be static, should I change it to make the whole class and all calls to it static? (not sure if my mock object would work then)
  • I'm not so happy with how the COMMON_NOTIFY_ADDRESSLIST event works, @HakanS do we know any plugin using that event? Should I change it as outlined in the todo comment?
  • this currently allows nested subscriptions. Eg. I can subscribe the * namespace and the wiki:* namespace. that doesn't really make sense. should we catch that (I think the original code might have)
added some commits Aug 12, 2012
@splitbrain first start at refactoring the subscription system BROKEN
This introduces a class for nicer wrapping and easier testing. Some
functions were changed to provide nicer APIs (no throwing around of
unescaped regexps) and to simplify things (hopefully).

The refactoring isn't completed yet, so this will break the subscription
system.

The goal is to move as much subscription related stuff to this class as
possible. Currently there is some code in lib/exe/indexer.php and maybe
elsewhere (common.php?). Additionally everything should be covered by
tests. A few tests are included here already.
2240ea1
@splitbrain handle empty changelog in getRecentsSince() e920a0a
@splitbrain more subscription refactoring BROKEN
now the actual sending of bulk messages (digest, list) is reimplemented
and partially tested.

Still not complete
adec979
@splitbrain added list test 8c7bcac
@splitbrain subscription system should work now again
This readds the last part of the subscription system: the normal "every"
subscriptions.
835242b
@splitbrain correctly check if subscriptions are enabled 84c1127
@splitbrain fixed subscription management
now adding and removing subscriptions works again
a0519fd
@splitbrain initialize new subscriptions with current time
We don't want to create a bunch of mails whenever a namespace is
subscribed. Only changes *after* the subscription should be considered.
This patch adds the timestamp to "every" style subscriptions as well,
though this data is ignored.
02308d1
@splitbrain minor cleanup f036cff
@adrianheine
Collaborator

I don't see a point in turning that into a class with only static methods (as you pointed out). Namespaces for poor PHP people?

@adrianheine adrianheine commented on the diff Aug 17, 2012
lib/exe/indexer.php
global $conf;
- if (!$conf['subscribers']) {
+ global $ID;
+
+ echo 'sendDigest(): started'.NL;
+ if(!actionOK('subscribe')) {
@adrianheine
Collaborator
adrianheine added a line comment Aug 17, 2012

Subscription::isenabled

@splitbrain
Owner
splitbrain added a line comment Aug 18, 2012

which does exactly the same, only that isenabled() is not a static function right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@adrianheine adrianheine commented on the diff Aug 17, 2012
inc/actions.php
$action = $params['action'];
// Perform action.
- if (!subscription_set($_SERVER['REMOTE_USER'], $target, $style, $data)) {
+ $sub = new Subscription();
+ if($action == 'unsubscribe'){
+ $ok = $sub->remove($target, $_SERVER['REMOTE_USER'], $style);
+ }else{
+ $ok = $sub->add($target, $_SERVER['REMOTE_USER'], $style);
+ }
+
+ if($ok) {
+ msg(sprintf($lang["subscr_{$action}_success"], hsc($INFO['userinfo']['name']),
+ prettyprint_id($target)), 1);
+ act_redirect($ID, $act);
+ } else {
@adrianheine
Collaborator
adrianheine added a line comment Aug 17, 2012

data parameter is not handled anymore.

@splitbrain
Owner
splitbrain added a line comment Aug 18, 2012

Yes, that was intentional. The current timestamp is set automatically in Subscription::add() now.

The previous implementation would allowed users to create a subscription with arbitrary data. For example one could create a subscription with a date way back in time, causing the wiki to create a huge change mail. Not sure if that's a real problem.

Was there any rationale behind having the data supplied from the frontend?

@michitux
Collaborator
michitux added a line comment Dec 16, 2012

The data parameter is set to the current time in subscription_handle_post(), not in the frontend. I think we should remove the data parameter from subscription_handle_post(), too. Either now or it should be deprecated. As the parameter isn't documented in the event documentation I assume it should be safe to remove it now.

@splitbrain
Owner
splitbrain added a line comment Jan 18, 2013

removed in 16c665d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@splitbrain consolidate more notification code in subscription class
This is untested and probably broken currently
2ed3803
@splitbrain
Owner

I do have some local unmerged changes to that branch and I recommend to wait with merging.

@splitbrain
Owner

I just checked what unpushed stuff I had there. I do have some local changes that move some of the notification stuff out of inc/common.php into inc/subscription.php - however this work isn't finished and currently broken. That's why I haven't pushed it yet.

The stuff in this pull request should work as is though. The question is should we merge this, knowing that my (currently unpushed) changes will later change the subscription class API again? Or should we wait til I got the whole thing done?

@HakanS
Collaborator

COMMON_NOTIFY_ADDRESSLIST is not used by any plugin I know of.

@michitux
Collaborator

COMMON_NOTIFY_ADDRESSLIST might not be handled by any plugin, but it is triggered by some plugins that want to send notifications like the discussion plugin, so yes, this affects plugins.

added some commits Nov 30, 2012
@splitbrain Merge branch 'master' into subscription
* master: (175 commits)
  some coding style improvements
  added .idea project folder to gitignore
  use correct setUp method and parent calls.
  Correct German plugin manager translation (download != install)
  correct return in sendDigest()
  Fix case-insensitive match in ACL checking
  GeSHi update to 1.0.8.11
  ignore empty header on mail sending
  remove empty BCC/CC mail headers
  Galician language update
  some welcome page changes
  Combine subsequent calls to strtr into a single transformation
  changed semicolon to colon in link to welcome page to make it less confusing
  fixed wrong sidebar showing in namespaces when sidebar is disabled
  Typo fix for TL;DR
  removed a bunch of outdated and irrelevant networking acronyms
  added another place to look for logo to make it more consistent (FS#2656)
  French language update
  Czech language update
  compat js findPosX/y more closely mimic historical function
  ...

Conflicts:
	inc/auth.php
	inc/common.php
	inc/subscription.php
	lib/exe/indexer.php
d14415e
@splitbrain fixed merge error in inc/auth.php
merged the wrong change here
10b5c32
@splitbrain moved registration notification to subscription class 790b772
@splitbrain
Owner

I moved a bit of stuff out of notify(). I'm still not 100% happy with it but is should be mergable again.

@splitbrain
Owner

https://github.com/dokufreaks/plugin-discussion/blob/master/action.php#L1165 relies on subscription_addresslist()

needs compatibility function

@michitux michitux commented on the diff Dec 16, 2012
inc/common.php
@@ -1113,82 +1113,26 @@ function notify($id, $who, $rev = '', $summary = '', $minor = false, $replace =
@michitux
Collaborator
michitux added a line comment Dec 16, 2012

$lang, $INFO and $DIFF_INLINESTYLES are unused globals in this function (they were previously used for the mail content).

@splitbrain
Owner
splitbrain added a line comment Jan 18, 2013

removed in cfbb991

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@splitbrain
Owner

merging postponed til @michitux is done with the review

@michitux michitux and 1 other commented on an outdated diff Dec 16, 2012
inc/subscription.php
}
- foreach (array('user', 'style', 'data') as $key) {
- if (!isset($pre[$key])) {
- $pre[$key] = '(\S+)';
+
+ /**
+ * Send the diff for some page change
+ *
+ * @param string $subscriber_mail The target mail address
+ * @param string $template Mail template, should be 'subscr_digest' or 'subscr_single'
@michitux
Collaborator
michitux added a line comment Dec 16, 2012

In notify() this parameter is used with the value mailtext.

@splitbrain
Owner
splitbrain added a line comment Jan 18, 2013

comment corrected in edf986a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@michitux michitux and 1 other commented on an outdated diff Jan 6, 2013
inc/subscription.php
+ public function add($id, $user, $style, $data = '') {
+ if(!$this->isenabled()) return false;
+
+ // delete any existing subscription
+ $this->remove($id, $user);
+
+ $user = auth_nameencode(trim($user));
+ $style = trim($style);
+ $data = trim($data);
+
+ if(!$user) throw new Exception('no subscription user given');
+ if(!$style) throw new Exception('no subscription style given');
+ if(!$data) $data = time(); //always add current time for new subscriptions
+
+ $line = "$user $style";
+ if($data) $line .= " $data";
@michitux
Collaborator
michitux added a line comment Jan 6, 2013

For the case when time() returned 0? I think after the if(!$data) $data = time(); above it can be assumed that $data is set to a valid value here.

@splitbrain
Owner
splitbrain added a line comment Jan 18, 2013

simplified in 004e7e6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@michitux michitux commented on the diff Jan 6, 2013
inc/subscription.php
+ $lastupdate = (int) $lastupdate;
+ if($lastupdate + $conf['subscribe_time'] > time()) {
+ // Less than the configured time period passed since last
+ // update.
+ continue;
+ }
+
+ // Work as the user to make sure ACLs apply correctly
+ $USERINFO = $auth->getUserData($user);
+ $_SERVER['REMOTE_USER'] = $user;
+ if($USERINFO === false) continue;
+ if(!$USERINFO['mail']) continue;
+
+ if(substr($target, -1, 1) === ':') {
+ // subscription target is a namespace, get all changes within
+ $changes = getRecentsSince($lastupdate, null, getNS($target));
@michitux
Collaborator
michitux added a line comment Jan 6, 2013

This only works when recent_days is configured to be large enough so the changes file still contains at least one change for each page in the namespace that has been changed since the last time subscriptions were sent. This could be a problem with namespace subscriptions for namespaces that are infrequently visited in a wiki that has lots of edits. Maybe we should scan the whole namespace for files that were modified more recently than $lastupdate when $lastupdate is smaller than time() - $conf['recent_days']*86400?

@splitbrain
Owner
splitbrain added a line comment Jan 18, 2013

@michitux I understand the problem description, but am not sure I understand your suggested fix. Where and how would you implement that? What do you mean by "scan the whole namespace"? Going through all individual changelogs?

@michitux
Collaborator
michitux added a line comment Jan 18, 2013

You don't need to read all changelogs, one can simply check if the page files themselves have been modified since $lastupdate which should be a lot faster (i.e. just a search-call with a filter on the modification date of the files). Then the changelogs of the pages that were found should be checked as the edits could be external.

@michitux
Collaborator
michitux added a line comment Jan 20, 2013

This doesn't need to fixed in this pull request as it's no regression, I've opened FS#2693 for this problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@michitux michitux and 1 other commented on an outdated diff Jan 6, 2013
inc/subscription.php
+ 'SUMMARY' => $summary,
+ 'SUBSCRIBE' => wl($id, array('do' => 'subscribe'), true, '&')
+ );
+ $hrep = array();
+
+ if($rev) {
+ $subject = 'changed';
+ $trep['OLDPAGE'] = wl($id, "rev=$rev", true, '&');
+ $df = new Diff(explode("\n", rawWiki($id, $rev)),
+ explode("\n", rawWiki($id)));
+ $dformat = new UnifiedDiffFormatter();
+ $tdiff = $dformat->format($df);
+
+ $DIFF_INLINESTYLES = true;
+ $dformat = new InlineDiffFormatter();
+ $hdiff = $dformat->format($df);
@michitux
Collaborator
michitux added a line comment Jan 6, 2013

The old code for this was

$DIFF_INLINESTYLES = true;
$hdf               = new Diff(explode("\n", hsc($old_content)),
                                          explode("\n", hsc($new_content)));
$dformat           = new InlineDiffFormatter();
$hdiff             = $dformat->format($hdf);
$hdiff             = '<table>'.$hdiff.'</table>';
$DIFF_INLINESTYLES = false;

and I don't see any reason why we don't need the hsc() anymore in order to prevent HTML injection into subscription emails. The old code introduced the $old/new_content variables in order to need only one rawWiki-call for the old/new revision.

@splitbrain
Owner
splitbrain added a line comment Jan 18, 2013

good catch. fixed in 903e04c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@michitux michitux and 1 other commented on an outdated diff Jan 6, 2013
inc/subscription.php
}
+ $hlist = '</ul>';
@michitux
Collaborator
michitux added a line comment Jan 6, 2013

This should be

       $hlist .= '</ul>';

(otherwise the whole list of links is lost).

@splitbrain
Owner
splitbrain added a line comment Jan 18, 2013

fixed in 8b87bc0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@michitux michitux and 1 other commented on an outdated diff Jan 6, 2013
inc/subscription.php
+ * @param string $subject The lang id of the mail subject (without the
+ * prefix “mail_”)
+ * @param string $context The context of this mail, eg. page or namespace id
+ * @param string $template The name of the mail template
+ * @param array $trep Predefined parameters used to parse the
+ * template (in text format)
+ * @param array $hrep Predefined parameters used to parse the
+ * template (in HTML format), null to default to $trep
+ * @return bool
+ */
+ protected function send($subscriber_mail, $subject, $context, $template, $trep, $hrep = null) {
+ global $lang;
+
+ $text = rawLocale($template);
+ $subject = $lang['mail_'.$subject].' '.$context;
+ $mail = new Mailer();
@michitux
Collaborator
michitux added a line comment Jan 6, 2013

The old code additionally set the from address, this was introduced after the subscription branch was created:

    $mail->from($conf['mailfromnobody']);

However this was only used for bulk subscriptions.

@splitbrain
Owner
splitbrain added a line comment Jan 18, 2013

readded in 29c964e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@michitux
Collaborator

I'm now done with the review, I think at least the last three things I've noted are major and should be fixed.

A more general question for me is the meaning of the Subscription class. Currently the class acts as a namespace for the subscription functions as the objects have no state. Something that would make more sense (semantically) for me is that a Subscription instance represents a certain number of subscriptions that could be used in the following way:

$subscription = new Subscription($id, $user, $style, $data);
$subscription->save();

or

$subscription = new Subscription($id);
$subscription->send_bulk();

or

$subscription = new Subscription($id, $user);
$subscription->getSubscriptions(); // maybe rather getSubscriptionData()?

The rest of the functions like the send_register() or the isenabled() function that don't fit into this concept could be static. Maybe also notify() could be included in the class as send_single() similar to send_bulk().

@splitbrain
Owner

Thanks for the review @michitux I'll look into the issues you found.

Yes, the class is currently not much more than a namespace (that's why I asked about making this all static in my first submission), but having it in a class made it a bit easier to unit test this. I'm not sure the API you suggest makes more sense, though. Using the constructor to add a new subscriber and to send mails to existing subscribers feels a bit wrong.

@michitux
Collaborator

I agree that my API isn't perfect, either. The idea was that an instance represents a collection of subscriptions, a bit like ORM just that we have one object for a collection of subscriptions. Maybe the functions for loading or creating subscriptions should rather be static and return an instance of Subscription(s)?

You are right concerning the testing part, that makes sense of course. I'm fine with merging this once the issues I've mentioned are fixed.

@splitbrain
Owner

All languages should be checked to make use of the @SUBSCRIBE@ placeholder in the mail files (related to #152)

@michitux

Now all subscription emails are sent with this sender, the behavior before was that this sender was only used for digest/list subscriptions, the other subscriptions were sent with the normal address which means that it was possible to configure for these emails that they are sent with the email of the logged in user (which is the author of the change) as sender. I'm not sure if there is any other purpose for the current user placeholder in the mailfrom setting.

@splitbrain splitbrain added a commit that referenced this pull request Jan 26, 2013
@splitbrain Merge branch 'subscription' Pull Request #125
* subscription: (25 commits)
  link directly to subscription management in mails
  only use mailfromnobody for bulk mails
  added missing context for list mails
  readded mailfromnobody to subscription sending
  correctly escape diffs in HTML mails
  fixed lists in HTML mails
  simplified subscription->add() code a bit
  comment adjusted
  removed unused vars
  removed data parameter in subscription_handle_post()
  fixed tests
  some reformatting
  added compatibility function
  moved registration notification to subscription class
  fixed merge error in inc/auth.php
  consolidate more notification code in subscription class
  minor cleanup
  initialize new subscriptions with current time
  fixed subscription management
  correctly check if subscriptions are enabled
  ...
ba56222
@splitbrain splitbrain merged commit c38b7fa into master Jan 26, 2013
@splitbrain link directly to subscription management in mails
This was updated in the english translation a while ago, but was still
missing in some translations
c38b7fa
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment