Configurable events #22

rowanthorpe · 2017-07-14T15:03:17Z

Inspired by the closing of #9 yesterday (and particularly the mention that recently Google seems to have fixed API support for non-birthday events), I checked that the API had in fact been fixed (it has!), and then I did a big refactor to configurably handle birthdays and anniversaries, and custom events (potentially more than one per contact, which required looping) with added labels per custom event. It has been running nicely for me under various test-runs (in English and Greek). I am very aware that @GioBonvi said

...I want to keep this project focused on birthdays only to avoid unnecessary complications for inexperienced users.

but I wanted this functionality for my own use anyway (even more than the birthday functionality). Therefore - now that the API handles it - I did these changes and made them as non-invasive as possible from the user-perspective (if users don't edit the default eventTypes array under OPTIONAL CUSTOMIZATION then it should behave the same as before, with the exception of some more generic wording in titles, to make sense for more than just birthdays). @GioBonvi I am posting the changes in this PR because:

When you see how nice (and non-invasive for users) it is you might change your mind :-)
I did a few other unrelated commits along the way which you will probably want either way. If you want them but not the non-birthday refactor commits then let me know which ones you want and I will rebase and PR them separately - I didn't wan't to spend time on that though until I was sure you don't want to just use all my commits as is.

If you want to include these changes then perhaps the repo could afford another rename (e.g. GoogleBirthdayNotifier -> GoogleContactsEventsNotifier) at some point, but before that maybe the changes should stay in a non-master ("devel"?) branch for a while until other translators have caught up with rewordings. If you don't want to include the non-birthday functionality I will maintain my changes as a patch-fork in my own repo, called GoogleContactsEventsNotifier.

GioBonvi · 2017-07-14T16:09:04Z

Hello @rowanthorpe, it's nice to hear from you again!

I've rapidly skimmed over your code and from what I could see you've done an oustdanding work - as usual ;-)
However, in these days I don't have the time to thoroughly analyze it and don't want to take hasty decisions, so I'll have to park this PR for now.

I will get back to you as soon as I manage to finish my exams, which should mean a week or so.

Thank you again!

rowanthorpe · 2017-07-14T16:36:42Z

OK, no rush. Good luck with the exams.

GioBonvi · 2017-07-21T09:41:26Z

code.gs

 *
- * Open up https://calendar.google.com, in the menu on the left click on the arrow next to the birthday calendar


Does this calendar have a default name?

In italian it's called "Compleanni ed eventi" which would translate into "Birthdays and events". I don't know the exact english version, but we should use it here.

Mine is presently set to "Birthdays & Anniversaries" but I am almost sure I've edited that name at some point, and Google seems to have no way to see the default pre-edited name. Are you able to temporarily change your settings to English to see what the default name becomes? Otherwise we should find another user with language set to English who knows they haven't edited the calendar name yet.

I think I might have edited It as well...
I'll try and have a look with my backup account tomorrow morning.

GioBonvi

Finally I have some time to address this!

In the end I've finally decided to implement all of your changes, because, as you said, they actually are really nice 😄

I've submitted some comments in my review about some minor problems which should be solved before merging.
I'd like to hear some feedback about the comment at line 99 as I don't know which way would be the best to make it the most user friendly.

When everything is fixed I'll proceed to merge into a development branch to update all the translations as per your suggestion.

Thank you again for the great work!

GioBonvi · 2017-07-21T09:49:32Z

code.gs

+// START OPTIONAL CUSTOMIZATION
+
+// Types of events to report, in a list. Can include one or more of 'BIRTHDAY', 'ANNIVERSARY', 'CUSTOM'
+var eventTypes = ['BIRTHDAY'];


I am thinking that something along these lines could be easier for users to setup:

var eventTypes = { BIRTHDAY: true, ANNIVERSARY: false, CUSTOM: false } eventTypes = Object.keys(eventTypes).filter(function(x) { return eventTypes[x] == true; });

What do you think?

Yes, I think making the editable part a hash/dict of explicit booleans as you suggest would be more friendly to non-techies (and self-documenting, so the comment-line can be removed). I think the conversion one-liner can be slightly simplified too due to the filter-method coercing the test-retval to boolean anyway, but maybe I'm just showing more familiarity with other languages than JS (please tell me if I'm assuming wrong for JS here). Let me know if the below looks good and I'll update the commit accordingly.

// START OPTIONAL CUSTOMIZATION var eventTypes = { BIRTHDAY: true, ANNIVERSARY: false, CUSTOM: false, } // END OPTIONAL CUSTOMIZATION //// other stuff... eventTypes = Object.keys(eventTypes).filter(function(x) { return eventTypes[x]; });

It's OK for me 👍

GioBonvi · 2017-07-21T09:49:39Z

code.gs

+
+              doLog('Contact # ' + i + ' ' + eventTypeNamePlural + '.');
+              contact = new Contact(event, eventType);
+              subjectBuilder.push(contact.fullName);


If a contact has two events in the same notification (e.g. birthday in 7 days and anniversary in 1 day) his/her name will be duplicated in the subject.

This could be prevented by collecting all the contacts names in an array, keeping them unique and building the subject after all the contacts have been processed.

GioBonvi · 2017-07-21T09:53:56Z

code.gs

+      var line;
+      line = ['\n', indent];
+      if (self.eventType === 'CUSTOM') {
+        line.push('{' + dateLabel + '} ');


I'd replace the {} around the custom event type with <>.
Affects line 590 as well.

GioBonvi · 2017-07-21T09:56:02Z

code.gs

+    self.dateLabels.forEach(function (dateLabel) {
+      var line, imgCount;
+      line = ['<li>'];
+      if (self.eventType === 'CUSTOM') {


I am planning to add a default "blank user" thumbnail for users without a profile photo: I'd move the custom event name after the photo so as not to disrupt the layout of the email (keeping all the thumbnails aligned vertically).

rowanthorpe · 2017-07-25T17:07:24Z

Hi, I was away from internet for a few days, but am back now, so will get a chance to look at this and make changes hopefully tomorrow, or the day after at the latest :-)

General renaming/rewording of var/function-names and comments in preparation for the upcoming refactoring, to hopefully make that diff easier to read. Also includes a small comment grammar-fix at line 344.

rowanthorpe · 2017-07-26T22:26:19Z

I just rebased the branch to address all the changes you suggested. For the naming of the calendar in the comments I used a wording which I think strikes a balance between "most likely default name" and "making it clear it might have a non-default name anyway". Of course if I missed something, made any presumptions you don't like, or need to do more changes, let me know.

GioBonvi

Except for this little thing everything is OK.

GioBonvi · 2017-07-27T09:40:23Z

code.gs

-          line.push(' - ');
+      // Custom label
+      if (self.eventType === 'CUSTOM') {
+        line.push('<' + dateLabel + '> ');


< and > need to be escaped in the HTML, otherwise the result is going to be interpreted as a tag and won't be rendered in the message.

Oh bugger, sorry I missed that (thanks for catching it), I tested it late last night in a hurry and only looked at the plaintext result when running test(). Fixing it and pushing in a minute...

This now configurably handles birthdays and anniversaries, and custom events (potentially more than one per contact) with added labels per custom event. Also includes one or two minor added newlines and improved spacing in plaintext email output (too much hassle to split to separate commit).

Remove unneeded entries, add TODOs

Use mailto: anchor for email address, remove redundant "send email now" link, remove unneeded line from translations.

GioBonvi · 2017-07-27T10:24:33Z

Does GitHub let you change the target branch for this PR form master to my repository's/configurable-events?
Otherwise I'll merge manually.

rowanthorpe · 2017-07-27T12:22:37Z

Yes, I just changed the target branch as requested.

rowanthorpe force-pushed the configurable-events branch 2 times, most recently from 9cf192e to f4acab4 Compare July 14, 2017 15:11

GioBonvi added the enhancement Improvements to existing bits of code. label Jul 19, 2017

GioBonvi reviewed Jul 21, 2017

View reviewed changes

GioBonvi requested changes Jul 21, 2017

View reviewed changes

Rename birthday -> event

7e9510c

General renaming/rewording of var/function-names and comments in preparation for the upcoming refactoring, to hopefully make that diff easier to read. Also includes a small comment grammar-fix at line 344.

rowanthorpe force-pushed the configurable-events branch from f4acab4 to b292f23 Compare July 26, 2017 22:21

GioBonvi requested changes Jul 27, 2017

View reviewed changes

rowanthorpe added 5 commits July 27, 2017 13:02

Fix space typo causing i18n-missed-key

597e2ae

Update Greek translations

1005b5d

Prepare non-Greek translations for update

1e5e14e

Remove unneeded entries, add TODOs

Link email address in html instead of separate link

2cd9efb

Use mailto: anchor for email address, remove redundant "send email now" link, remove unneeded line from translations.

rowanthorpe force-pushed the configurable-events branch from b292f23 to 2cd9efb Compare July 27, 2017 10:07

GioBonvi approved these changes Jul 27, 2017

View reviewed changes

rowanthorpe changed the base branch from master to configurable-events July 27, 2017 12:22

GioBonvi merged commit c614dec into GioBonvi:configurable-events Jul 27, 2017

rowanthorpe deleted the configurable-events branch July 27, 2017 14:34

GioBonvi mentioned this pull request Jul 29, 2017

New release - new name: any ideas? #31

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configurable events #22

Configurable events #22

rowanthorpe commented Jul 14, 2017

GioBonvi commented Jul 14, 2017

rowanthorpe commented Jul 14, 2017

GioBonvi Jul 21, 2017

rowanthorpe Jul 26, 2017

GioBonvi Jul 26, 2017

GioBonvi left a comment

GioBonvi Jul 21, 2017

rowanthorpe Jul 26, 2017

GioBonvi Jul 26, 2017

GioBonvi Jul 21, 2017

GioBonvi Jul 21, 2017

GioBonvi Jul 21, 2017

rowanthorpe commented Jul 25, 2017

rowanthorpe commented Jul 26, 2017

GioBonvi left a comment

GioBonvi Jul 27, 2017

rowanthorpe Jul 27, 2017

GioBonvi commented Jul 27, 2017

rowanthorpe commented Jul 27, 2017

		*
		* Open up https://calendar.google.com, in the menu on the left click on the arrow next to the birthday calendar

Configurable events #22

Configurable events #22

Conversation

rowanthorpe commented Jul 14, 2017

GioBonvi commented Jul 14, 2017

rowanthorpe commented Jul 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GioBonvi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rowanthorpe commented Jul 25, 2017

rowanthorpe commented Jul 26, 2017

GioBonvi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GioBonvi commented Jul 27, 2017

rowanthorpe commented Jul 27, 2017