FreedomCoop / valuenetwork

Fork coming from NRP-Sensorica to use and work for FREEDOM COOP
http://fair.coop
GNU Affero General Public License v3.0
31 stars 12 forks source link

Make membership request comments more visible to Coop Admins #128

Closed bhaugen closed 7 years ago

bhaugen commented 7 years ago

The comments app looks like it offers a lot of possibilities.

Here's the documentation. The quotes below are from the doc.

We can list both the number of comments for a membership request, and the comments themselves, wherever we want:

For example, in a blog entry page that has a variable named entry, you could use the following to load the number of comments:

{% get_comment_count for entry as comment_count %}.

Displaying comments To display a list of comments, you can use the template tags render_comment_list or get_comment_list.

Quickly rendering a comment list The easiest way to display a list of comments for some object is by using render_comment_list:

{% render_comment_list for [object] %} For example:

{% render_comment_list for event %} This will render comments using a template named comments/list.html, a default version of which is included with Django.

We could also list all comments on a page in the coop admin app, with filters for date range.

Comments send a lot of signals that we can catch and do something with, including when a new comment was posted. For example, when the first comment is posted by a new user, we could either send an email notification to all the coop admins, or put an announcement somewhere on the coop admin app.

XaviP commented 7 years ago

I'm appending this to work/models.py:

# Connecting signal "comment_was_posted" to membership_comment_notification()
from django.contrib.comments.models import Comment
from django.contrib.comments.signals import comment_was_posted
comment_was_posted.connect(membership_comment_notification, sender=Comment)

def membership_comment_notification(sender, comment, **kwargs):
    msr = comment.content_object
    if msr.agent:
        msr_owner_name = msr.agent.name
        msr_owner_email = msr.agent.email
    else:
        msr_owner_name = msr.name
        msr_owner_email = msr.email_address

Some doubts:

bhaugen commented 7 years ago

@XaviP - you asked:

How can I get all admins of "FreedomCoop Membership Request Group" for sending the notification?

I don't think we have an association type for admins. What I have used for freedom coop admins is just everybody who has access to the coop admin app. You can get them like this:

admins = [agent for agent in EconomicAgent.objects.all() if agent.is_staff()]

If that gets you the people you want, I could encapsulate that in a manager method that you could call more easily.

Which notification system I have to use? The django one or the one you implemented?

I don't know of a django notification system. Do you mean this? https://docs.djangoproject.com/en/1.8/ref/contrib/messages/

The one I have been using is an open source app created by the Pinax gang: django-notification

[Edit: here's the pypi page: https://pypi.python.org/pypi/django-notification/0.1.5 django-notification is not maintained anymore. We adopted it in NRP way back when it was being maintained, and it still works for us, so we have not changed. The pinax gang has moved to https://github.com/pinax/pinax-notifications ]

It sends email notifications. Any new notification needs some setup, which is a little cranky. If you want to use that one, let me know and I'll do the setup.

Edit: Is work/models.py the right place to connect comment_was_posted?

Depends on what you want to do in membership_comment_notification. If just send email notifications, then that seems ok. If do something in the coop admin app, then someplace in valueaccounting.

XaviP commented 7 years ago

What I have used for freedom coop admins is just everybody who has access to the coop admin app.

I see problems here when the number of membership requests and comments increase a lot. If we are talking about 100 membership requests with 2 or 3 comments in each, this could be very annoying for not-admins of "FreedomCoop Membership Request Group". Then,

django-notification is not maintained anymore.

I don't understand. It seems that pinax-notifications is in 3.0.1 version: https://djangopackages.org/packages/p/pinax-notifications/ https://github.com/pinax/pinax-notifications

and django-notifications is pointing to the same github repo as pinax-notifications: https://github.com/pinax/django-notification

Can we update pinax-notifications to 3.0.1?

bhaugen commented 7 years ago

@XaviP I think pip pulls from that pypi link which has a tar file.

We're not using pinax-notifications. We could update to that, but I don't know what all would be involved and how long it would take.

[Edit: pinax-notifications was based on their earlier repo called django-notification (singular) and may be so similar that it would not take much work.]

XaviP commented 7 years ago

We already got a notification system. It's in your settings (on the top-right user dropdown menu). Look for notification settings. They are now all opt-out, but I think we can change that to opt-in.

How can I setup a new notification system item, like "Comments in Freedom Coop Membership Requests" ?? Can you point me in the code an example of how it's done and how I have to indicate it when sending the notification?

We can also set up an admin association type and make the people who should get these notifications admins of whatever group is responsible for whatever the notification is about.

I think that would be necessary when implementing join-request, so maybe we can make a generic way to send notifications if "Freedom Coop Membership Requests" group has this association type done. Is it easy for you to do it?

bhaugen commented 7 years ago

How can I setup a new notification system item, like "Comments in Freedom Coop Membership Requests" ?? Can you point me in the code an example of how it's done and how I have to indicate it when sending the notification?

Here's an overview: https://pypi.python.org/pypi/django-notification/0.1.5 We're not using the notice lists in the website, so neither of the .html templates is needed. But everything else:

If you want me to create the notice types and templates, give me a list of what you want and I'll do it. Or if you want me to pair program in telegram or appear.in or something, I'm around and available.

bhaugen commented 7 years ago

@XaviP what's your timing on this? I got two jobs on my plate now, help you with this, or create the new Project model. Both are paused.

I can go on to another issue on the sprint list if you're not ready yet and we're undecided on the new app.

XaviP commented 7 years ago

Don't pause for me. Go ahead. No new app is already decided. It's ok.

bhaugen commented 7 years ago

I think we have decided by default not to add a new app this week. If you disagree, I can still do it now, but pretty soon I will pile up more code to move over...

bhaugen commented 7 years ago

In other words, I created a new project branch and started to work in the work app.

XaviP commented 7 years ago

I AGREE, NO PROB ;-)

bhaugen commented 7 years ago

Copied from my comment up-thread: @XaviP I just noticed this in pinax-notifications: apparently we are not supposed to use post_sync_db anymore (although we still are). Also still seems to be working. We switched to post_migrate in models.py, which also seems to work. But pinax-notifications says we are actually supposed to be using AppConfig.. I'll check it out tomorrow.

I started checking into AppConfig. Here's the 1.8 django doc about it: https://docs.djangoproject.com/en/1.8/ref/applications/

I am bewildered. It will take me a week to sort that out and figure out what if anything to do about it. So I am thinking I'll test just replacing post_sync_db with post_migrate, which seems to work fine.

Anybody disagree?

XaviP commented 7 years ago

Hey @bhaugen, all versions of django-notification when making pip list in different enviroments (prod ocp, test ocp, dev enviroment) are 1.3.3, not 0.1.5 So I think it's already updated. And it's working ok, isn't it?

bhaugen commented 7 years ago

@XaviP

all versions of django-notification when making pip list in different enviroments (prod ocp, test ocp, dev enviroment) are 1.3.3, not 0.1.5

Yup. I picked the wrong pypi page to cite. Should be https://pypi.python.org/pypi/django-notification/1.3.3

So I think it's already updated. And it's working ok, isn't it?

Yes, it's working. But it's called notification (singular), whereas pinax.notifications is plural.

bhaugen commented 7 years ago

@XaviP I'm done with the Other field for now. Should I start working on this? Do the notification setup?

If so, I'll start a new branch for it, unless you already have one.

XaviP commented 7 years ago

Yes, it's working. But it's called notification (singular), whereas pinax.notifications is plural.

The package in requuirements.txt is in singular: https://github.com/FreedomCoop/valuenetwork/blob/master/requirements.txt#L11 So, it's updated and working. We can close this conversation about updating this notification package, right?

Should I start working on this? Do the notification setup?

I'm working on it but not pushed until stable. I think we are duplicating work.

bhaugen commented 7 years ago

I'm working on it but not pushed until stable. I think we are duplicating work.

Nope. I never started working on comment notifications. It's all yours. But if you want some help or anything, let me know.

bhaugen commented 7 years ago

We can close this conversation about updating this notification package, right?

Yes that part of tmhe conversation is over.

XaviP commented 7 years ago

Pushed the skeleton of comment notification:

Not working yet. Needs to include the owner of membersr and to filter the user who comment to not recieve email.

XaviP commented 7 years ago

@bhaugen Do you know how can I debug sended emails in local? By the way, thanks for pointing me to the code where to config the subscription.

bhaugen commented 7 years ago

Do you know how can I debug sended emails in local?

I've done it, but it ain't pretty. Give me a few mins to refresh my memory...

bhaugen commented 7 years ago

This will take you some part of the way. In shell_plus, do:

import base64
from django.utils.six.moves import cPickle as pickle
batches = NoticeQueueBatch.objects.all()
queued_batch = batches[0]
queued_batch.pickled_data
notices = pickle.loads(base64.b64decode(queued_batch.pickled_data))
notices
bhaugen commented 7 years ago

More:

In your virtualenv, go into lib/python2.7/sitepackages/notification

The magic starts in engine.send_all() and continues in models.send_now() and finished in backends.email.deliver()

You can throw a import pdb; pdb.set_trace() somewhere in there and from the commandline, do

./manage.py emit_notices 

and step through the magic.

Hope that all made some sense...

XaviP commented 7 years ago

From shell_plus seems that everything is ok, thanks! Pending add the creator of the membership request to the send-to list. I have a doubt about this: when the candidate is not agent yet, I can get the user object filtering by "requested_username". But when the user is already agent (that is it has an agent object id in field agent in membership_request) how can I get the user object?

bhaugen commented 7 years ago

If you got an agent, you can say

agent.user().user

If you got a user, you can say

user.agent.agent

I could encapsulate that stuff better so it is not so verbose, but those will work.

[edit: however, in most cases, the agent and the user have the same email address. and in both cases it is called just email. So agent.email or user.email.]

XaviP commented 7 years ago

Thanks! Pull request to merge into master: https://github.com/FreedomCoop/valuenetwork/pull/135 If nobody see problems, I'll merge tomorrow to test with real emails.

XaviP commented 7 years ago

Tested in testocp. Everything seems ok. Closing this issue. If any improvement needed we can open a more concrete issue.