implement email functionality in workflow

[Martijn Faassen]

We need to implement basic email functionality for the workflow.

[Martijn Faassen (faassen)]

This has now been deferred until after the release.

[Kit Blake (kitblake)]

Sub-feature: the emails should have links to corresponding
documents appended to the message. 'Documents' is plural,
because if an Author/Editor does a bulk request/approve, we
don't want to spam them (a la cvs) but add a list of links
to affected documents. Gets tricky with different Authors....

[Martijn Faassen (faassen)]

Deferring until after membership revision.

[Clemens Robbenhaar (crobbenhaar)]

The CVS messages seems to indicate
Martijn is actually working on this right now,
thus I set this to "in-progress".

 Martijn: is this ready to test or are there more
fixes to come?

[Martijn Faassen (faassen)]

This is now ready to 'test', though it needs lots of
more tweaking. I just did a checkin; read checkin
message for more details.

[Martijn Faassen (faassen)]

But I'll leave this on in-progress. This feature is hard to
test if you don't have an actual email host and your
users have actual email addresses. Right now I'm faking it,
but some new membership stuff will be landing in the Silva
core soon hopefully that will allow users to set email
addresses, which should take care of that.

[Clemens Robbenhaar (crobbenhaar)]

The IMemberService specifies:

    def get_member(userid):
        """Get member object for userid, or None if no such
member object.
        """

  ... and the implementation usually conform to this.
This will lead to a lot of bugs e.g. when deleting a user:
usually the calling places assume to get something
which is not None.

 It seems the tests currently fail for that reason,
as the test user is not a known member.

 Maybe one should fix the callers, but as there
is some "Unknown user" object, why do not change
the spec and return this one object instead?
 I guess this is much less error prone.

 Any other opinions?

[Kit Blake (kitblake)]

Testing the email (via the shell) there's something strange
with the sending. If an editor revokes approval of a
document, the message is not sent immediately. Only when
another action happens, such as the author requests approval
(for the same or another document), then is the message
sent. But it arrives last, which would make the author think
the editor instantly unapproved it.

[Martijn Faassen (faassen)]

Looks like a bug; some code may not be calling
'send_pending_messages()' when it should.

[Martijn Faassen (faassen)]

I fixed the bug; it is now sending pending messages when
revoking approval in the publish tab of a folder. Please
test.

[Clemens Robbenhaar (crobbenhaar)]

I have a security related question:
The "set_XXX" methods in the Simple/Extended membership
are protected as "ChangeSilvaContent".

 Shouldn't read this "ChangeSilvaAccess"
Currently Authors may change the address, etc of any member
TTW directly (They cannot change the email TTW only because this has no
docstring).

 Secend stupid question: Is someone work-in-progress for some simple ZMI
pages for the "Member"s? Changing email addresses via typing in URL's like
/silva/Members/test_author/set_email?<email address hidden>
is not really fun.

 There is a simple "add" but no "edit".
 If nobody does work on this one, I will make a first try, but I do not
want to do double work ...

[Clemens Robbenhaar (crobbenhaar)]

... forgot my third question, sorry:

 Can I please change the "print" statements to use
zLOG.LOG instead? In my installation, printing to stdout get lost
somewhere, but the zLOG works nicely.

 Or is there any reason to avoid the zLOG?

[Guido Wesdorp (guido-infrae)]

Clemens Klein-Robbenhaar wrote:

> I have a security related question:
>The "set_XXX" methods in the Simple/Extended membership
>are protected as "ChangeSilvaContent".
>
> Shouldn't read this "ChangeSilvaAccess"
>Currently Authors may change the address, etc of any member
>TTW directly (They cannot change the email TTW only because this has no
>docstring).
>
Not sure why I set the restrictions so low, will look into that later...

> Secend stupid question: Is someone work-in-progress for some simple ZMI
>pages for the "Member"s? Changing email addresses via typing in URL's like
>/silva/Members/test_author/set_email?<email address hidden>
>is not really fun.
>
>
There already is a Silva view defined for that: go to
<silva_root>/Members/<memberid>/edit and you can do just that.

[Clemens Robbenhaar (crobbenhaar)]

> > Second stupid question: Is someone work-in-progress for some simple ZMI
 > >pages for the "Member"s? Changing email addresses via typing in URL's like
 > >/silva/Members/test_author/set_email?<email address hidden>
 > >is not really fun.
 > >
 > >
 > There already is a Silva view defined for that: go to
 > <silva_root>/Members/<memberid>/edit and you can do just that.

 Actually I have to skip the '/edit', but then it works.
Thanks and sorry, I did not track the changes of the last week carefully
enough.

[Guido Wesdorp (guido-infrae)]

Clemens Klein-Robbenhaar wrote:

> Actually I have to skip the '/edit', but then it works.
>Thanks and sorry, I did not track the changes of the last week carefully
>enough.
>
>
You're right, I made it a public view... Forgot that myself as well :)

[Clemens Robbenhaar (crobbenhaar)]

> You're right, I made it a public view... Forgot that myself as well :)

The public "ExtendedMember" view looks very funny in my custom
layout_macro.html ;-)

Would it be possible to have an "edit" view instead, thus it keeps
inside the SMI look?
Or is this page meant to be displayed to visitors, too?

In the latter case I would favor to have two views, one "edit"
view to be used in the SMI, and one public view for visitors from the
outside. (If I am the only one thinking so, I will do that in my custom
local version, thus You do not have to mbother about meintaining two
near identical views.)

[Guido Wesdorp (guido-infrae)]

Clemens Klein-Robbenhaar wrote:

>Would it be possible to have an "edit" view instead, thus it keeps
>inside the SMI look?
>Or is this page meant to be displayed to visitors, too?
>
>
>
I think I actually chose for this setup because the member objects are
stored in a plain Zope folder
instead of in a Silva one, so there are probably all sorts of weird
acquisition issues, but I'm not
100% sure about this. Will do some checking as soon as I have the time...

Guido

[Clemens Robbenhaar (crobbenhaar)]

Another issue: I propose to point the url
in the generated email to the "/edit/tab_preview" of the object.

I guess this is the place the editor having to decide about the request
will look first.

In case of a rejection the author getting the message will most probably
want the edit-tab, which is only one click away in this case.

If no one objects, I will check this in tomorrow.

More issues with request approval via the containers publish tab:

 - in this case a publication date is required;
   in the case of a "VersionedContent" publish tab it is not.
   I guess the latter makes more sense
   (Actually the publish date cannot really be reused by
   the Editor deciding about the request).

   I propose to fix this by not requiring a publish darte field.
   One could even omit the publish and expiration date fields for
   authors; everything they fill in will be lost in the workflow anyway.

   (An alternative is to assume the date is part of the request for
   approval. In this case the values filled in should be redisplayed as
   default values if the editor visits the publish tab. This is quite
   hard to achieve for the containers publish tab.

 - a little annoyance in case of the bulk request:
   the mail contains a list of messages, where the messages
   only differ by the URL of the content object.

   Yes, I see there is code which should ensure there are not send N
   messages, thus the current state is already an improved one ;-)
   I just wounder if it would be possible to handle this in a different
   way. Maybe the original interface I have worked out to the "request
   for approval" needs to be reworked to request/reject/approve
   a lot of content objects with only one message.

   The last issue is a non-constructive one, which means I have no idea
   how to fix this yet. As a corollary I would not complain if this is
   not getting fixed ;)

 - just another one: if the content is not "published now",
   maybe one can append the publication date to the generated message.

 - any arguments pro or contra filling the "From:" field
   of the mail with the mail address of the actor (I.e. the
   Author requesting approval or the editor rejecting the
   request/approving the content?

   I can think of:
      Pro: allows simple feedback just by email reply
      Contra: some people will not like to have programms sending
        automatically generated mails with thier "From:" header.

   The contra argument may be handled by setting the "Reply-To" header
   instead.

[Martijn Faassen (faassen)]

Clemens Klein-Robbenhaar wrote:
> ... forgot my third question, sorry:
>
> Can I please change the "print" statements to use
> zLOG.LOG instead? In my installation, printing to stdout get lost
> somewhere, but the zLOG works nicely.
>
> Or is there any reason to avoid the zLOG?

Go ahead. I presume ZLOG writes it still ends up in stdout as well if
Zope's started in debug mode.

Regards,

Martijn

[Martijn Faassen (faassen)]

Guido Wesdorp wrote:
> > Secend stupid question: Is someone work-in-progress for some simple ZMI
> >pages for the "Member"s? Changing email addresses via typing in URL's like
> >/silva/Members/test_author/set_email?<email address hidden>
> >is not really fun.

> There already is a Silva view defined for that: go to
> <silva_root>/Members/<memberid>/edit and you can do just that.

This is a membership user interface issue to me; there needs to be a
clear way to get to your member page. There is now a link to it
in the bottom, but I don't think that's enough -- the whole membership
click flow needs to be reviewed before the release. You don't want users
to end up in 'dead end' pages with no way to get out, for instance.

Regards,

Martijn

[Martijn Faassen (faassen)]

Clemens Klein-Robbenhaar wrote:
> Clemens Klein-Robbenhaar <email address hidden> added the comment:
> > You're right, I made it a public view... Forgot that myself as well :)
>
> The public "ExtendedMember" view looks very funny in my custom
> layout_macro.html ;-)
>
> Would it be possible to have an "edit" view instead, thus it keeps
> inside the SMI look?
> Or is this page meant to be displayed to visitors, too?

I think it should be edit myself. Do we have any reasons to make it
public? Users are chief editor on their own member object, after all,
which implies they can do things in a Silva edit screen.

> In the latter case I would favor to have two views, one "edit"
> view to be used in the SMI, and one public view for visitors from the
> outside. (If I am the only one thinking so, I will do that in my custom
> local version, thus You do not have to mbother about meintaining two
> near identical views.)

I agree it should be this way. If you're interested in working on this
please coordinate this with Guido (otherwise we'll try to figure it out
here ourselves).

Regards,

Martijn

[Martijn Faassen (faassen)]

Clemens Klein-Robbenhaar wrote:
> Clemens Klein-Robbenhaar <email address hidden> added the comment:
>
> Another issue: I propose to point the url
> in the generated email to the "/edit/tab_preview" of the object.
>
> I guess this is the place the editor having to decide about the request
> will look first.
>
> In case of a rejection the author getting the message will most probably
> want the edit-tab, which is only one click away in this case.
>
> If no one objects, I will check this in tomorrow.

Agreed, go ahead.

> More issues with request approval via the containers publish tab:
>
> - in this case a publication date is required;
> in the case of a "VersionedContent" publish tab it is not.
> I guess the latter makes more sense
> (Actually the publish date cannot really be reused by
> the Editor deciding about the request).

> I propose to fix this by not requiring a publish darte field.

Agreed. Will you do this?

> One could even omit the publish and expiration date fields for
> authors; everything they fill in will be lost in the workflow anyway.

I didn't realize things were lost in the workflow. Originally authors
could suggest a publication datetime/expiration time. Possibly this never
worked correctly but the intent was that editors can view this if it's
supplied and modify it if desired.

> (An alternative is to assume the date is part of the request for
> approval. In this case the values filled in should be redisplayed as
> default values if the editor visits the publish tab. This is quite
> hard to achieve for the containers publish tab.

True. For the container publish tab this can't really work and I imagine
the best user interface is to ignore the author's suggestions.. Possibly
this means the best strategy is to remove the ability for authors to
supply this information altogether... Would simplify the user interface
for authors.. Let's debate this a bit further on this issue. :)

> - a little annoyance in case of the bulk request:
> the mail contains a list of messages, where the messages
> only differ by the URL of the content object.
>
> Yes, I see there is code which should ensure there are not send N
> messages, thus the current state is already an improved one ;-)
> I just wounder if it would be possible to handle this in a different
> way. Maybe the original interface I have worked out to the "request
> for approval" needs to be reworked to request/reject/approve
> a lot of content objects with only one message.

You mean the messages should say something like:

Author asked for approval of:

  * foo/bar/baz
  * qux/foo/bar
  * ..

That would indeed be nicer. But rather hard to implement before the release,
I suspect.. Any ideas?

> The last issue is a non-constructive one, which means I have no idea
> how to fix this yet. As a corollary I would not complain if this is
> not getting fixed ;)

It'd just need a rather bigger overhaul of the code..

> - just another one: if the content is not "published now",
> maybe one can append the publication date to the generated message.

Sounds good.

> - any arguments pro or contra filling the "From...

[Clemens Robbenhaar (crobbenhaar)]

[...]
 > > One could even omit the publish and expiration date fields for
 > > authors; everything they fill in will be lost in the workflow anyway.
 >
 > I didn't realize things were lost in the workflow. Originally authors
 > could suggest a publication datetime/expiration time. Possibly this never
 > worked correctly but the intent was that editors can view this if it's
 > supplied and modify it if desired.
 >

 I did not implement this in the first version of the "request for
approval" and forgot to mention that this has not been done. Only
rediscovered the missing feature recently.

 > > (An alternative is to assume the date is part of the request for
 > > approval. In this case the values filled in should be redisplayed as
 > > default values if the editor visits the publish tab. This is quite
 > > hard to achieve for the containers publish tab.
 >
 > True. For the container publish tab this can't really work and I imagine
 > the best user interface is to ignore the author's suggestions.. Possibly
 > this means the best strategy is to remove the ability for authors to
 > supply this information altogether... Would simplify the user interface
 > for authors.. Let's debate this a bit further on this issue. :)

 I agree, which makes a debate a little difficult ;-)
If authors want to supply a publish date, they maybe want to argue about
that date, and in this case they need to go to the contents tab anyway.
 The same applies for Editors when doing the review.

 Hm, some authors may complain their editors ignore their suggestions as
the editor lazily makes bulk publishing via the container tab at the end
of the day ... however in this case maybe the workflow is braindead
anyway, as it seems the editor does not to any review.

 Hm, I just remember why I need the containers publish tab at all.
Usually this happens when I want to bring a complete folder with all its
contents public at once. This happened only if this is the initial
version, or the container has been closed completely due to some renaming
or copying previously. In this case the request for approval workflow
does not apply anyway.

 Lengthier discussion without providing any value ;)
I tend to ignore the publication date provided by the authors in case
the editor does a bulk publish via the containers tab, unless someone
comes up with a use case which makes some sense not to drop this information.

 [...]
 > You mean the messages should say something like:
 >
 > Author asked for approval of:
 >
 > * foo/bar/baz
 > * qux/foo/bar
 > * ..

 Yes, something in that way.
 >
 > That would indeed be nicer. But rather hard to implement before the release,
 > I suspect.. Any ideas?
 >
 > > The last issue is a non-constructive one, which means I have no idea
 > > how to fix this yet. As a corollary I would not complain if this is
 > > not getting fixed ;)
 >
 > It'd just need a rather bigger overhaul of the code..
 >

 Yes, it is a design flaw from my first implementation. I did not take
the case of sending mails around into account. Instead I only supplied to
read the message in the "publish" tab of the corresponding content.
 Thus the messages ar...

[Clemens Robbenhaar (crobbenhaar)]

There are some modifications which may lead to nicer mailing
(having a Subject: and Reply-To: header set up properly.

 Attempts to implement user friendly, or any error handling in case of
trouble with sending mail failed so far, but the "agreed" subissues
from msg1275 should be fixed now.

 Some stupid wish: can we move the "EmailService" class out of the
"SimpleMember.py"? It seems this class is only very loosely related to
the other ones in this file, and before the release it is not to late to
make such moves ...

 Hm, the state of the issue is actually "testing"; this has not changed
for sure my the latest cvs commits ...

[Martijn Faassen (faassen)]

Guido is moving EmailService out of SimpleMember.py.

[Guido Wesdorp (guido-infrae)]

i've moved the emailservice... can anybody tell me what the
exact issue is here? i got a little lost, and don't know
whether this can be set to resolved or not...

[Clemens Robbenhaar (crobbenhaar)]

After removing my ancient 'service-message' ad 'Member' folder and
refresh everything seems to work.

 There are some minor issues about the people who are (possibly)
notified; I do not know if the current implementation is ok,
or if it creates too much spam.

 A minor inconsistency: When someone else but the last author
requests approval for a content object, the last author is notified, too.
However the last author is not notified, if the latter person withdraws
the request for approval.
 Should I fix this?

[Martijn Faassen (faassen)]

We're assuming this one works so resolving. Clemens, I recall
seeing some checkin messages along the lines of changing
who gets notified; does this refer to your last entry
on this issue? (the 'should I fix this?' question)

Anyway, good enough for the beta, resolving.