send emails to users that are mentioned in bucket comments #195

jlopker · 2017-12-16T23:27:09Z

No description provided.

chime-mu · 2018-01-18T00:43:34Z

@jlopker The email that's being sent look like this

 Michael Arnoldus mentioned you in the bucket Test 4 in debug3.

<p><html><body><p>Dette er en besked der nævner <a href="localhost:9000/#/users/5094" name="5094">@Michael</a> <br></p></body></html></p>

So the HTML is being displayed, not actually interpreted as HTML.

The source in the mail is this

<p>
  Michael Arnoldus mentioned you in the bucket <a href=3D"http://localhos=
t:9000/#/buckets/1757">Test 4</a> in debug3.
</p>

<p>
  &lt;p&gt;&lt;html&gt;&lt;body&gt;&lt;p&gt;Dette er en besked der n=C3=A6=
vner &lt;a href=3D&quot;localhost:9000/#/users/5094&quot; name=3D&quot;50=
94&quot;&gt;@Michael&lt;/a&gt;=C2=A0&lt;br&gt;&lt;/p&gt;&lt;/body&gt;&lt;=
/html&gt;&lt;/p&gt;

</p>

jlopker · 2018-01-18T22:53:21Z

@chime-mu oh that's very odd! is that how the emails are showing up in mailcatcher? for me, they're showing up normal...

chime-mu · 2018-01-19T07:02:21Z

@jlopker That is indeed very odd. Could you take a look at the mail source?

I'm wondering if this is connected to my message including 8-bit/non-ascii characters?

jlopker · 2018-01-20T00:58:33Z

@chime-mu i ended up getting that same problem when I pulled down the code again.. very strange. but i think i fixed it now. let me know if that's working for you.

chime-mu · 2018-01-26T11:01:21Z

@jlopker Tested once more.

The mail now looks good. However there's still a few things not working.

Basic markdown (italic italic and bold) doesn't work anymore.
The protocol has changed (more below)
I've given some remarks to the code - not sure if you can see them?

Protocol changes

In the version of cobudget in the master branch the API regarding comments (POST'ing as well as GET'ing) the body is always pure markdown. I think that's a great solution, because it makes the API useful for different kind of frontends.

The version in this branch sends something in between. Several <br> and   are transmitted and received, so now it's a mix between markup and HTML.

Also, when posting an @-mention, it's HTML <a href="/users/5093" name="5093">@Bum</a> but the response and following GET is markup [@Bum](/users/5093) which makes the protocol inconsistent.

jlopker · 2018-01-30T21:58:51Z

@chime-mu

I can't see the comments you wrote in the code...

So I'm struggling a bit to figure out how to make this work. If when you @mentioned someone, the mention was converted to markdown, it would use the bracket/parentheses markdown link style. I had this at first, but everyone agreed it looked too nerdy. So I changed it to look like a normal link (with an tag) but this causes the mixing of the html and markdown. To combat this, I convert the html to markdown on the backend before it gets saved. Do you have another idea of how this could work? Other option I explored was to not do any styling of the @-mentions on the front end and have the backend search for @'s and convert them to links. but the problem with this is that we don't have usernames and often the names we're using have spaces. So it makes it really hard to figure out what the "right" name is and then search on it.

What do you think? Is there another obvious solution that i'm missing?

chime-mu · 2018-01-31T19:46:48Z

@jlopker

The architecture that makes sense to me is that the backend and frontend(s) has a fairly stable protocol. Two reasons:

To in general reduce complexity of the codebase and keep it maintainable, it's quite important that we keep low coupling between different components. It should be possible to figure the backend code out without knowledge about how the frontend is coded and vice versa. This is is done using a clearly defined protocol that is as simple as possible.
It should be possible to build a different frontend, which could be web-based (like what the food network guys want to do) or could be a slack integration, a chatbot or whatever. This also requires a known an well-specified protocol.

Up to this point, the body part of the comments has been in pure markdown. This is not as well-specified as I prefer, since there exist different definitions of markdown, but it's good enough in my view. By mixing HTML and markdown in becomes quite unclear what the content of the body field can be - and this increases coupling between backend and frontend - and makes it harder for us and others to make a different frontend.

So I agree the the @-mention should look like a link. That's what users expect. And the it should be converted to mark-down in the frontend.

This is in general a good way to divide responsibility. The frontend decides how things should be entered, edited and displayed. That's a frontend responsibility. The backend receives, stores, manipulates, filters and finds well-defined data.

Does this make sense?

jlopker · 2018-02-01T02:18:17Z

@chime-mu

hmm I don't think I explained myself well enough.. the problem is with the display in the text box and with getting all the necessary information to the backend (we need the user id for everyone that was @-mentioned). I don't see a clear way to do this if we use markdown exclusively. if this doesn't make sense, let's hop on the phone to chat about it!

chime-mu

@jlopker

I'm thinking if the frontend can send <a href="http://localhost:9000/users/5093" name="5093">@Bum</a> it could just as well send [@Bum](user://5093) (or something similar).

That said there might very well be something I'm not seeing clearly, so let's hop on the phone and talk about it.

I'll send you a doodle with a few choices later today.

chime-mu · 2018-02-01T08:59:18Z

app/controllers/comments_controller.rb

+      bucket = Bucket.find(comment_params[:bucket_id])
+      user_links.each do |userId|
+        user = User.find(userId)
+        UserMailer.notify_member_that_they_were_mentioned(author: current_user, member: user, bucket: bucket, body: email_body).deliver_now


We should use deliver_later instead of deliver_now

ah yes i did that for testing purposes and forgot to revert! change pushed!

jlopker · 2018-02-01T19:28:03Z

That's what I was doing initially but the problem with that is that using that syntax would make [@Bum](user://5093) appear in the textbox on selection of a mentioned person. When I originally demoed that solution in mid-December (maybe you remember?), the feedback was that that was too nerdy. Which led me to do the whole complicated <a> tag workaround, which, to be clear, I agree is not clean. But herein is where I am stuck.. eek

jlopker · 2018-02-08T18:43:09Z

@chime-mu this is ready for another review! i formatted the links as [@Bum](uid:443). this requires a bit of finding and replacing to generate the correct links on both the front end and the backend. not sure if the way i implemented it is the best way possible so if you could take an extra close look at that that would be great! thanks!

jlopker · 2018-02-08T19:39:15Z

@chime-mu errr ok so this is weird.. when I test with deliver_now the email shows up in my mailcatcher exactly as I'd want it, with the html interpolated. But when I test deliver_later, the html shows as not interpolated. I searched around to see if anyone else is having this problem but can't find anything- everything just talks about how deliver_later is just asynchronous deliver_now. Do you have any ideas about why this could be happening?

chime-mu · 2018-02-08T22:16:27Z

@jlopker That sounds really weird. Happy to test and take a look first thing tomorrow :)

chime-mu · 2018-02-21T16:03:08Z

@jlopker The link to the user in the mail doesn't work when I click in firefox. This is tested on localhost, so not sure if the same problem be there when fully deployed. However it's probably a good idea to make it work when testing on localhost as well.

chime-mu · 2018-02-21T16:06:36Z

app/controllers/comments_controller.rb

+        user = User.find(userId)
+        UserMailer.notify_member_that_they_were_mentioned(author: current_user, member: user, bucket: bucket, body: email_body).deliver_later
+      end
+    end


I wouldn't put the code that converts from markdown to HTML in the controller, but rather in the user_mailer code that generates the mail.

@chime-mu It's not always the case that the user_mailer would get called though. If no one is mentioned in the comment then no mails would get sent. Because of this, I'm not sure that it makes sense to me to put this code in the user_mailer. Does that make sense? Or is there something I'm not seeing?

Why would you convert to HTML if no mail should be sent?

jlopker · 2018-02-22T19:48:03Z

@chime-mu What do you mean the link doesn't work? Like you click on it and then nothing happens?

jlopker added 5 commits December 16, 2017 12:48

send emails to users that are mentioned in bucket comments

d398c8e

email text updates

6e6d157

copy change, and deliver_now changed to deliver_later

26d44b2

change url to use action mailer config

6d1df21

change to deliver_later

de691e6

make email body text html_safe

a87bd8b

Merge branch 'master' into feature/at-mention

3005099

chime-mu suggested changes Feb 1, 2018

View reviewed changes

deliver_now -> deliver_later

a19e93f

backend comments recieve only markdown now

33dc9a6

change to deliver_later

81f5bb6

jlopker and others added 2 commits February 8, 2018 14:50

remove reverse_markdown as it's not in use

60c5353

Fixed HTML rendering problem

33a2d51

chime-mu reviewed Feb 21, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

send emails to users that are mentioned in bucket comments #195

send emails to users that are mentioned in bucket comments #195

jlopker commented Dec 16, 2017

chime-mu commented Jan 18, 2018

jlopker commented Jan 18, 2018

chime-mu commented Jan 19, 2018

jlopker commented Jan 20, 2018

chime-mu commented Jan 26, 2018

jlopker commented Jan 30, 2018

chime-mu commented Jan 31, 2018

jlopker commented Feb 1, 2018 •

edited

Loading

chime-mu left a comment

chime-mu Feb 1, 2018

jlopker Feb 1, 2018

jlopker commented Feb 1, 2018 •

edited

Loading

jlopker commented Feb 8, 2018

jlopker commented Feb 8, 2018

chime-mu commented Feb 8, 2018

chime-mu commented Feb 21, 2018

chime-mu Feb 21, 2018

jlopker Feb 22, 2018

chime-mu Feb 26, 2018

jlopker commented Feb 22, 2018

send emails to users that are mentioned in bucket comments #195

Are you sure you want to change the base?

send emails to users that are mentioned in bucket comments #195

Conversation

jlopker commented Dec 16, 2017

chime-mu commented Jan 18, 2018

jlopker commented Jan 18, 2018

chime-mu commented Jan 19, 2018

jlopker commented Jan 20, 2018

chime-mu commented Jan 26, 2018

Protocol changes

jlopker commented Jan 30, 2018

chime-mu commented Jan 31, 2018

jlopker commented Feb 1, 2018 • edited Loading

chime-mu left a comment

Choose a reason for hiding this comment

chime-mu Feb 1, 2018

Choose a reason for hiding this comment

jlopker Feb 1, 2018

Choose a reason for hiding this comment

jlopker commented Feb 1, 2018 • edited Loading

jlopker commented Feb 8, 2018

jlopker commented Feb 8, 2018

chime-mu commented Feb 8, 2018

chime-mu commented Feb 21, 2018

chime-mu Feb 21, 2018

Choose a reason for hiding this comment

jlopker Feb 22, 2018

Choose a reason for hiding this comment

chime-mu Feb 26, 2018

Choose a reason for hiding this comment

jlopker commented Feb 22, 2018

jlopker commented Feb 1, 2018 •

edited

Loading

jlopker commented Feb 1, 2018 •

edited

Loading