Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Direct messages #2186

Merged
merged 17 commits into from Dec 4, 2017
Merged

Direct messages #2186

merged 17 commits into from Dec 4, 2017

Conversation

@deivid-rodriguez
Copy link
Contributor

@deivid-rodriguez deivid-rodriguez commented Nov 10, 2017

馃帺 What? Why?

Implements direct messages between users.

馃搶 Related Issues

馃搵 Subtasks

  • Empty conversation index page.
  • Full conversation index page.
  • Empty conversation page.
  • Full conversation page.
  • Reply box.
  • Unread counts.
  • Links to the feature.
    • Global menu.
    • Proposal author

馃摲 Screenshots (optional)

Conversation list

anotherlist

Conversation page

conversation

Unread counts

unread

Link to feature

link to feature

馃懟 GIF

sumo

@ghost ghost added the in-progress label Nov 10, 2017
@deivid-rodriguez deivid-rodriguez changed the title Direct messages [WIP] Direct messages Nov 10, 2017
@deivid-rodriguez deivid-rodriguez force-pushed the feature/direct_messages branch from def9885 to 05f3734 Nov 11, 2017
@codecov
Copy link

@codecov codecov bot commented Nov 11, 2017

Codecov Report

Merging #2186 into master will increase coverage by 0.01%.
The diff coverage is 99.33%.

@@            Coverage Diff             @@
##           master    #2186      +/-   ##
==========================================
+ Coverage    98.6%   98.61%   +0.01%     
==========================================
  Files        1251     1272      +21     
  Lines       28825    29271     +446     
==========================================
+ Hits        28422    28865     +443     
- Misses        403      406       +3

@deivid-rodriguez deivid-rodriguez force-pushed the feature/direct_messages branch 4 times, most recently from 64353c2 to 6df5ae8 Nov 17, 2017
@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Nov 17, 2017

Hei, I'm basically done here. I posted a few screenshots!

I'm thinking of linking to the feature from proposal authors and from comment authors for now, what do you think? Also, I'm now sure why but I chose the name "chat" internally, "discussion" sound more participation-ish, right? Is it worth that I rename it?

@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Nov 20, 2017

Adding the link to the comment component is a bit tricky, I'm tempted to call this done and ship this initial version with only a message link next to the proposal author. What do you think?

@deivid-rodriguez deivid-rodriguez changed the title [WIP] Direct messages Direct messages Nov 21, 2017
@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Nov 21, 2017

Decided to call this "done" for a first MVP. Ready for review.

Copy link
Contributor

@josepjaume josepjaume left a comment

Nice job! Please check my comments. Also - I think this is asking for a mail to be sent when a new message is received. Otherwise, it could go unnoticed to the user.

I get how adding a "message" link can be tricky in the comments section. How about linking to the user profile (we should create a simple one) and having a message button there? It would make more sense to me from a UX point of view as well.

module Decidim
module Messaging
module ChatHelper
def link_to_current_or_new_chat_with(user)
Copy link
Contributor

@josepjaume josepjaume Nov 21, 2017

Can you add documentation?

module Decidim
module Messaging
def self.table_name_prefix
"decidim_messaging_"
Copy link
Contributor

@josepjaume josepjaume Nov 21, 2017

Is this needed?

Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Nov 21, 2017

No. It simplifies associations inside the namespace so one does not need to specify class names and foreign keys if they follow conventions. But it's not used anywhere else, so I should probably remove it, right?

Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Nov 21, 2017

Hei, so I had at look into this and it is actually necessary, so that the table names of the different models are properly resolved.

At this point we could do several things:

  • Don't namespace table names. That means changing the migrations so that the tables are named with just the prefix decidim_ instead of decidim_messaging_.

  • Move everything to a separate engine. In that case, doing isolate_namespace Decidim::Messaging in the engine definition would accomplish the same thing I'm doing here.

  • Leave it as it is.

I vote for the last one :)


delegate :mark_as_read, to: :receipts

def self.start!(originator:, interlocutors:, body:)
Copy link
Contributor

@josepjaume josepjaume Nov 21, 2017

Can you add documentation?

chat
end

def self.start(originator:, interlocutors:, body:)
Copy link
Contributor

@josepjaume josepjaume Nov 21, 2017

Can you add documentation?

chat
end

def add_message(sender:, body:)
Copy link
Contributor

@josepjaume josepjaume Nov 21, 2017

Can you add documentation?

messages.last
end

def unread_count(user)
Copy link
Contributor

@josepjaume josepjaume Nov 21, 2017

Can you add documentation?


module Decidim
module Messaging
class Message < ApplicationRecord
Copy link
Contributor

@josepjaume josepjaume Nov 21, 2017

Missing docs


validate :sender_is_participant

def envelope_for(recipients)
Copy link
Contributor

@josepjaume josepjaume Nov 21, 2017

Missing docs


module Decidim
module Messaging
class Participation < ApplicationRecord
Copy link
Contributor

@josepjaume josepjaume Nov 21, 2017

Missing docs


module Decidim
module Messaging
class Receipt < ApplicationRecord
Copy link
Contributor

@josepjaume josepjaume Nov 21, 2017

Missing docs

@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Nov 21, 2017

Ooops, I forgot to add docs indeed! Thanks for pointing that out @josepjaume! I'll add them in a bit.

Regarding the other stuff:

  • Yes, I thought about email notifications but I was expecting to leave and discuss that in another PR. Maybe one email notification per message is too much, maybe we should make it configurable... I prefer to leave that discussion for another time and leave it as it is for now. I added "In-app" notifications and unread counts so the feature is still useful as long as you're engaged in the platform. :)

  • Regarding the public profile page, yes, that sounds like the best idea. But is there a design mockup or something at least? Again, it seems to me like something different enough to warrant a separate PR.

@josepjaume
Copy link
Contributor

@josepjaume josepjaume commented Nov 21, 2017

@deivid-rodriguez I agree on that the profile page is something to discuss later (as it has privacy implications). Not so sure about the email thing though - I think it provides a critical value to the feature - users tend to live their lifes outside the platform, and we want to drag them in.

Maybe the @decidim/product people have something to say about it?

@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Nov 21, 2017

Fair enough, let's wait for feedback and I'll proceed as @decidim/product thinks is best! Just note that I'm in favor of email notifications for this feature, I'm saying that this first MVP could live without them.

@deivid-rodriguez deivid-rodriguez force-pushed the feature/direct_messages branch from 6df5ae8 to 1e6f91f Nov 21, 2017
@andreslucena
Copy link
Member

@andreslucena andreslucena commented Nov 21, 2017

@deivid-rodriguez I also think that it'd need to have email notification.

For the moment, the best thing would be to have an email by every message. On the future we can discuss about having multiple messages by email, or having it configurable through the user profile page.

@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Nov 21, 2017

Ok, I'll add them then!

@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Nov 21, 2017

Two things:

  • What do you think about sending an email notification only on the first unread message for each conversation? I think it's pointless if a user sends three messages in a row, to send 3 separate emails.

  • Also, can you give me a raw copy of the email? I guess we should not include the body of the message, but just a link to the conversation?

@deivid-rodriguez deivid-rodriguez force-pushed the feature/direct_messages branch 2 times, most recently from d9cd5a7 to ea1ae23 Nov 22, 2017
@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Nov 22, 2017

I implemented email notifications as I suggested in the previous message. Regarding the copies, I just added a raw first version, feel free to tweak it as you please.

This is ready for a second review pass!

@mrcasals
Copy link
Contributor

@mrcasals mrcasals commented Nov 22, 2017

@deivid-rodriguez what is the "Link to feature" screenshot showing? I can't see anything different 馃槙

@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Nov 22, 2017

Hahaha, yes, it's a tiny difference. It's just a little envelope next to the proposal's author that links to the conversation with her.

Copy link
Contributor

@mrcasals mrcasals left a comment

Looks awesome to me! 馃帀

let(:date) { Time.zone.now - 1.year }

it "returns the full date" do
expect(helper.simple_date(date)).to eq("02.06.44")
Copy link
Contributor

@mrcasals mrcasals Nov 22, 2017

Separation with dots? Isn't that weird?

Also, I've added a similar date to this in #2206 (decidim-core/config/locales/en.yml), but with "%d/%m/%Y" format (note the order), we should come to an agreement with the order

Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Nov 22, 2017

Not sure, I copied it from my telegram list 馃ぃ. It uses different formats according to how close the message is in time.

Copy link
Contributor

@mrcasals mrcasals Nov 22, 2017

I mean the case when the date is from another year, you use month.day.year and I'm using day/month/year in my PR

Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Nov 22, 2017

About the order, I think it's a confusion between british notation (little endian) and american notation (middle endian). Yeah, we should clarify which type of English en.yml is meant to hold for us. I vote for the british version which is more readable for us. If you agree, I'll change it!

Copy link
Contributor

@mrcasals mrcasals Nov 23, 2017

I'd go with British version, yeah 馃榿

Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Nov 23, 2017

I fixed it, nice catch by the way!

@@ -7,6 +7,7 @@ class ChatsController < Decidim::ApplicationController
helper Decidim::DatetimeHelper

before_action :authenticate_user!
before_action :load_chat, only: :show
Copy link
Contributor

@mrcasals mrcasals Nov 22, 2017

Instead of this, what I usually do is define a helper_method :chat:

def chat
  @chat ||= Chat.find(params[:id])
end

So in your views you can use the method, which will raise if you make a typo in it (instance variables do not raise error if undefined).

I'll leave it to you though, I don't want to impose my format 馃槤

Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Nov 23, 2017

Yeah, you're right! And it's consistent with other controllers, so let me change that!

@message ||= chat.add_message(sender: sender, body: form.body)
end

def notify_interlocutors
Copy link
Contributor

@mrcasals mrcasals Nov 22, 2017

Why not use the Decidim::EventManager here? Too verbose maybe?

Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Nov 22, 2017

I'm not sure how this EventManager works, I checked how this type of emails are sent in other places and tried to do it similarly...

Copy link
Contributor

@josepjaume josepjaume Nov 29, 2017

Hi @deivid-rodriguez! Sorry to chime in so late. You can find an example of events working here:

https://github.com/decidim/decidim/blob/master/decidim-comments/app/commands/decidim/comments/create_comment.rb#L46

and here:

https://github.com/decidim/decidim/blob/master/decidim-comments/app/events/decidim/comments/comment_created_event.rb

Basically, an Event is an abstraction that creates a notification or sends an email behind the scenes, depending on which module is included (Decidim::Events::EmailEvent or Decidim::Events::NotificationEvent).

Do you think you could switch to use that? It's a really good use case for notifications :)

Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Nov 29, 2017

Ok, will do!

Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Nov 29, 2017

Thanks for the pointers, by the way. It makes sense to use that indeed.

Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Nov 29, 2017

Ok, so I had a look at this and I'd rather leave it as it is because:

  • There's no other example where these Event's don't create a notification but only send emails. Other places where only emails need to be sent (joining meetings, devise, report/moderation email alerts) use mailers directly (not a very good reason per se, but it's the reason why I implemented it this way initially).
  • Current EmailNotificationGenerator does not support conditionally sending emails upon certain conditions on the recipients and the base resource, which I'm doing here.
  • I'm not sure it would make the code better in this case. It introduces an indirection that feels like unnecesarily complicating things to me.

Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Nov 30, 2017

After a second thought, I guess I can do the recipient filtering before publishing the event, so the second point wouldn't be a blocker. But I still think this introduces an unnecessary level of complexity for little gain. Anyways, let me know and I'll update this as you please.

Copy link
Contributor

@josepjaume josepjaume Nov 30, 2017

Well, the main idea here is to have a visual indication somewhere that you have unread messages, as well as receiving an email notification. But now that I give it a second thought, maybe the nature of that notification is different.

Facebook, for example, uses two different icons in the top bar to indicate unread conversations and events. Maybe we should treat them differently and do something similar in Decidim?

We can of course do this in a further PR, but I think it's quite valuable to have this on the first iteration.

To be clear, I mean something like:

image

Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Nov 30, 2017

@josepjaume Note that I'm already doing that in this PR. There's a separate indicator for messages in the top menu that works just like the notification indicator: it gets hightlighted if there are new unread messages. Have a look at the screenshots!

Copy link
Contributor

@josepjaume josepjaume Nov 30, 2017

OMG, I totally missed that. Looks like you were a step ahead of me all the way along 馃憤

@deivid-rodriguez deivid-rodriguez force-pushed the feature/direct_messages branch 3 times, most recently from b13a984 to 0488605 Nov 23, 2017
@xabier
Copy link
Member

@xabier xabier commented Nov 30, 2017

@josepjaume @deivid-rodriguez ok then, we have a conversation module, made of messages, we use the term message for each message, and conversation for the component/module/feature

@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Nov 30, 2017

Almost, except that Conversation is not the name of the module per se, but the name of the main class inside the module.

To try to further clarify this, each entry in the first screenshot represents a Conversation, but each entry in the second screenshot represents a Message.

Also, note that there's not currently a separate component/module for this feature, it lives in core under a namespace that I've called Decidim::Messaging.

@xabier
Copy link
Member

@xabier xabier commented Nov 30, 2017

thanks for clarifying!

@mrcasals
Copy link
Contributor

@mrcasals mrcasals commented Dec 1, 2017

So @deivid-rodriguez is this blocked by something, or can this be merged? (there are conflicts, though)

@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Dec 1, 2017

I need to fix conflicts and do the renaming we have agreed on chat -> conversation. I'll do it later today. Other than that it's ready!

@mrcasals
Copy link
Contributor

@mrcasals mrcasals commented Dec 1, 2017

Awesome, thanks! We'll wait for this before releasing 0.8.0 then 馃榿

@deivid-rodriguez deivid-rodriguez force-pushed the feature/direct_messages branch from 9952f2d to 331b500 Dec 1, 2017
@josepjaume
Copy link
Contributor

@josepjaume josepjaume commented Dec 4, 2017

Nice!

@josepjaume josepjaume merged commit f9161ef into master Dec 4, 2017
19 checks passed
@ghost ghost removed the in-progress label Dec 4, 2017
@josepjaume josepjaume deleted the feature/direct_messages branch Dec 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants