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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add messaging to order (closes #327) #350

Merged
merged 1 commit into from
Apr 10, 2015
Merged

Conversation

paroga
Copy link
Member

@paroga paroga commented Feb 18, 2015

No description provided.

@wvengen
Copy link
Member

wvengen commented Feb 18, 2015

Yay, very good idea!

Messaging is done in a plugin, so this would need to go into a deface override. See this example.

@@ -40,6 +40,15 @@ def group_id=(group_id)
add_recipients Group.find(group_id).users unless group_id.blank?
end

def order_id=(order_id)
@order_id = order_id
unless order_id.blank?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this check?

@paroga
Copy link
Member Author

paroga commented Feb 18, 2015

ad deface) i know, but I wasn't able to get it working :-/
ad check) yes, it's not necessary any more

@wvengen
Copy link
Member

wvengen commented Feb 18, 2015

What did you try with deface?

@paroga
Copy link
Member Author

paroga commented Feb 18, 2015

create a file in orders/show folder with similar content of your example

@wvengen
Copy link
Member

wvengen commented Feb 18, 2015

Ok, I'm trying now with deface - and run into the old bug where :javascript in the original breaks it - spree/deface#125. Solution would be to move the javascript in orders/show.html.haml to a separate file. I'm on it.

@wvengen
Copy link
Member

wvengen commented Feb 18, 2015

Oh, and why not making the "x Ordergroups" a messaging link instead of the mail button? This would be in-line with other user names being a link to send a message.

@paroga
Copy link
Member Author

paroga commented Feb 18, 2015

Yes, that was exactly the error I got.
The "x Ordergroups" "expands" to the list of Ordergroups. How do you want to handle that then?

@wvengen
Copy link
Member

wvengen commented Feb 18, 2015

That's the title attribute, a hyperlink could go to the messaging screen.

@paroga
Copy link
Member Author

paroga commented Feb 18, 2015

If I do it that way I get some kind of "layering problem": a create_mail_link_for_ordergroups function should only need a list of order groups as parameter. The message page requires the order_id instead and passing a list of order groups/users via URL does not seam like a perfect solution for me either.

@wvengen
Copy link
Member

wvengen commented Feb 18, 2015

I like the design pattern that info displayed on the screen is also a way to interact with it. In this way: seeing the ordergroups allows one to do something with it - sending them a message.

I think the technical problem of how to create the link is solvable. We can still use the order_id (since that can be used to get the same list of ordergroups).

@wvengen
Copy link
Member

wvengen commented Apr 3, 2015

Thanks for your work! Testing this, I'm a bit confused on the message page, since it shows no addressee. I see a number of options:

  • Include all recipients in the recipients field
  • Add a special field "Order ..." to the Groups field
  • Replace the "Order ..." (and recipients?) fields with "Order: ...."
  • Allow to put members, groups and orders in the recipients input field (something I'd like to do in the future).

@paroga
Copy link
Member Author

paroga commented Apr 9, 2015

at least on my installation it adds all recipients to the recipients field. any idea? i had some "browser caching" problems at the beginning too, but they were gone later :-/

@wvengen
Copy link
Member

wvengen commented Apr 10, 2015

Ok, I'll try again. The code looks ok in that respect.
If you feel like it, you might want to git pull && git rebase master

@order_id = order_id
for ordergroup in Order.find(order_id).ordergroups
add_recipients ordergroup.users
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be shorter and more performant

add_recipients Order.find(order_id).ordergroups unless order_id.blank?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Silly me, that wouldn't work at all. Perhaps another has_many :through in Order would work, but let's leave it this way :o

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I couldn't resist. If you like:

# in app/models/order.rb
has_many :ordergroups, :through => :group_orders
has_many :users_ordered, :through => :ordergroups, :source => :users
# in plugins/messages/app/models/message.rb#order_id=
add_recipients Order.find(order_id).users_ordered if order_id

@wvengen wvengen merged commit 124f77f into foodcoops:master Apr 10, 2015
@paroga paroga deleted the order_message branch March 11, 2016 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants