Readd time to mail for non-expiry activities

[pako81]

In #1118 we removed the time from the notification emails because of the confusion generated by the activity time for shares expiry.

However, this caused the timestamp to be removed for all activity notifications. This PR adds logic for having the correct date format depending on the activity type (expiration vs non-expiration).

[phil-davis]

I added a commit to remove some special drone actions from CI - we are cleaning up various things that need special privileges/tokens and the needed tokens no longer exist. Since this PR has just been created and is failing because of this, I may as well fix it here and get a good CI result.

[phil-davis]

https://drone.owncloud.com/owncloud/activity/3427/6/7
The workflow that gathers unit test coverage pushes the individual coverage results into an S3 cache and the SonarCloud coverage analysis runs with the data from that cache. The cache token(s) are in the process of being sorted out. so the unit test coverage fails for now.

[jvillafanez]

I'm not sure about the implementation.
If the idea is to target one or two specific events, you could do something like if ($activity['amq_subjectparams'] === 'shareExpired') {....} (not sure if we can use a constant)
While it can be hard to find an event that contains "shareExpired" which we don't want to format appropriately, it could "accidentally" happen. I mean, false positives could happen.

In order to target a group of events, you'll have to distinguish those events somehow. I guess we could use a $activity['eventType'] === 'expiringEvent' or even better $activity['dateFormat'] === 'formatDateRelativeDay' so it's the actual activity's sender the one deciding the formatting.

[pako81]

Yes, I also thought about targeting a group of events but did not find a reliable way. getItemsForUser is used for getting all items for the user we want to send an email to from the oc_activity_mq table https://github.com/owncloud/activity/blob/master/lib/MailQueueHandler.php#L147 and this is how the table looks like:

+-------------------+---------------+------+-----+---------+----------------+
| Field             | Type          | Null | Key | Default | Extra          |
+-------------------+---------------+------+-----+---------+----------------+
| mail_id           | bigint(20)    | NO   | PRI | NULL    | auto_increment |
| amq_timestamp     | int(11)       | NO   | MUL | 0       |                |
| amq_latest_send   | int(11)       | NO   | MUL | 0       |                |
| amq_type          | varchar(255)  | NO   |     | NULL    |                |
| amq_affecteduser  | varchar(64)   | NO   | MUL | NULL    |                |
| amq_appid         | varchar(255)  | NO   |     | NULL    |                |
| amq_subject       | varchar(255)  | NO   |     | NULL    |                |
| amq_subjectparams | varchar(4000) | NO   |     | NULL    |                |
+-------------------+---------------+------+-----+---------+----------------+

So what we get at https://github.com/owncloud/activity/blob/master/lib/MailQueueHandler.php#L253 is just an array of those fields. So the only valid way to filter for expiry events seemed to use the amq_subjectparams field.

[jnweiger]

Good to see progess here!
I'd like to include this for the 10.13.0 release - see #1176 - please retarget this PR into the release-2.7.2 branch if you agree.

[pako81]

Just noticed that public links expiration activities contain the following in amq_subjectparams:

[{"1715":"\/public"},"auto:automation"]

while expiration for users/group shares has:

[{"1714":"\/New folder"},"auto:automation","shareExpired"]

so the PR is in any case incomplete as with the code in place we are missing the case for public links expiration.

Maybe we can try to strpos the string automation which is included in both cases, or an alternative approach would be to use amq_subject in the if condition: users/group shares expiration has unshared_by while public links have link_expired.

However, note that unshared_by is also used for the share recipient notifications when the sharer manually unshares the resource. So it will not be unique to the expiry event only.

[jvillafanez]

The amq_subjectparams is json-encoded (

activity/lib/Data.php

Line 158 in ce947de

'subjectparams' => \json_encode($event->getSubjectParameters()),

) so we should treat it as json, not a plain string.

What about including new column for options, or something like that? We could have a column with a bit mask, for example:

• 1 -> use datetime instead of date for the amq_timestamp
• 2 -> format short date instead of long date
• 4 -> format short time instead of long time
  Assuming we use a 32-bit integer, we still have margin to add additional options later.

For the migration, we can assume that all the current data will have a 0 value as option, which means there won't be changes.

[IljaN]

IIRC there is/was a policy that we only add new columns on major release. Something that needs to be discussed.

[pako81]

@jvillafanez it looks like the activity files hooks already include the info about the share being or not expired: https://github.com/owncloud/activity/blob/master/lib/FilesHooks.php#L358

What about adding a new column expiry (or whatever name better suits) to oc_activity_mq which could have the value 0 or 1 depending if we are dealing or not with an expire event and then use date or datetime in MailQueueHandler.php based on the value of that column.

Note: this would additionally require core changes.

[jvillafanez]

I think that's more or less the idea I've proposed.

Taking into account that we might not be allowed to add new columns into the tables freely, I think we should try to make it count.
Adding the expire column is fine, but I think it only fits for this particular use case. For any other similar use case we'll need to make more changes and likely add new columns or tables. I'd prefer to have something that could be used in more use cases even if we only implement this one right now.