liqd / thentos

A tool for privacy-preserving identity management (PPIM)
Other
56 stars 9 forks source link

Basic support to send emails [#472] #473

Closed np closed 8 years ago

np commented 8 years ago

No support yet for the HTML optional body.

np commented 8 years ago

This ready for a first round of reviews. I still have some issues with my setup, so I've not ran the tests myself.

fisx commented 8 years ago

Notes from the phone call:

afterthough: ./docs/messaging.md still contains a few fixmes, and i think most of them are things that can be designed using nothing but common sense and general knowledge. @np, could you add that either here or in a separate PR?

ChristianSi commented 8 years ago

afterthough: ./docs/messaging.md still contains a few fixmes, and i think most of them are things that can be designed using nothing but common sense and general knowledge. @np, could you add that either here or in a separate PR?

There is one FIXME about the return value of the new endpoint. I proposed

{"data": {}}

which, with some hindsight, doesn't make any sense. In your PR, the return value is Post '[JSON] () which (I think) corresponds to HTTP status code 204 No Content and no body. I think this is very reasonable and the messaging.md should be adapted to document it.

fisx commented 8 years ago

Clarification:

  • "recipient" can be "persona", "email", "group". how those are distinguished still needs to be designed.

I meant PersonaId personaExternalUrl here. This is available to the service and the default way to refer to a persona. (EDIT: the service uses external urls as a handle, thentos internall uses persona ids. Thanks @ChristianSi for correcting me.)

fisx commented 8 years ago

Nice to have: send email to ServiceGroup.

There are Groups of users (organising things in thentos) and ServiceGroups of personas (organising the same things on the service side, i.e. adhocracy). See ./docs/terminology.md for more details.

The service may want to send email to all (transitive) members of a service group. For this to work, I think there may be data types, transactions and actions missing involving the use (not creation and assignment) of service groups. Currently, we have:

grep persona_groups thentos-core/schema/schema.sql thentos-core/src/Thentos/Transaction.hs
grep group_tree thentos-core/schema/schema.sql thentos-core/src/Thentos/Transaction.hs

Please ping me if you want me to elaborate on this.

fisx commented 8 years ago

Updated earlier comment. Now:

I meant PersonaId personaExternalUrl

np commented 8 years ago

Hum, thanks to git rebase the date of this commit should have been now and not yesterday.

np commented 8 years ago

This is ready for another round of review @fisx @ChristianSi

fisx commented 8 years ago

Looks good. Just a few comments.

np commented 8 years ago

While my last commit ec0071a might be better for incremental review, I'm going to rebase onto master to make it a proper PR.

np commented 8 years ago

Actually I rebased but preserved the separate commits at least for now.

fisx commented 8 years ago

thanks for keeping the individual commits! losing them would have been annoying.

generally speaking, i was asked not to rebase in between incremental reviews, and now i see why :-). i'll assume though that the old commits haven't changed and will just read the last one.

fisx commented 8 years ago

I made two small commits. Could you check if you have any objections and otherwise merge?

np commented 8 years ago

LGTM