search

DMPbelgium / roadmap

5 stars 1 forks source link

notification emails are not sent to new collaborator when sharing a plan #88

Closed StCyr closed 4 months ago

StCyr commented 9 months ago

Please complete the following fields as applicable:

What version of the DMPRoadmap code are you running? (e.g. v2.2.0)

All

Expected behaviour:

A notification email should be sent to the new collaborator when sharing a plan

Actual behaviour:

No notification email is sent

Steps to reproduce:

  1. Open a plan
  2. Go to the "Share" section
  3. Invite a new collaborator by entering his/her email address
  4. Select whatever permission you like
  5. Click submit
  6. Confirm that no emails are received by the new collaborator (though the application reports no error and says "Invitation to xxxxxx@yyyyy issued successfully")
StCyr commented 9 months ago

something to do with https://github.com/scambra/devise_invitable ?

nicolasfranck commented 9 months ago

Are these environment variables set?

DMP_SMTP_ADDRESS=0.0.0.0
DMP_SMTP_PORT=25

They are used here

I tested this against a local mail catcher (mailhog) that opens an smtp port at on port 1025, and with a gui on http port 8025. It holds the mails in memory. And the mails were caught.

If the environment is set, I can only guess. The local smtp server does not forward the mails to an outgoing server? I do remember that, if it cannot send any mails, the application would crash (which does not happen). So the mails are received by an smtp server from where they are probably stuck, or are blocked.

StCyr commented 9 months ago

nope, there's really nothing in the mail server logs when this notification should be sent. While there are mail server logs when I remove someone's role to a plan.

Are these environment variables set?

DMP_SMTP_ADDRESS=0.0.0.0 DMP_SMTP_PORT=25

DMP_SMTP_ADDRESS is set, but not DMP_SMTP_PORT. Should be using the default port...

nicolasfranck commented 9 months ago

Just a stupid test, but try to set the port to something that does not exist, and see if it crashes? If it does crash, then we know for sure that it successfully send the previous mail, and the search should be in the further smtp flow ..

StCyr commented 9 months ago

will try that

StCyr commented 9 months ago

In the mean time, I made a tcpdump of test.dmponline.be traffic while sending an invite from the console:

irb(main):010:0> root@dmponline:/opt/roadmap-test/config# docker exec -it roadmap-test /bin/bash
root@roadmap-test:/opt/roadmap# 
root@roadmap-test:/opt/roadmap# export EXECJS_RUNTIME=Disabled
root@roadmap-test:/opt/roadmap# export RAILS_ENV=production
root@roadmap-test:/opt/roadmap# bin/rails console
Copying Bootstrap glyphicons to the public directory ...
Copying TinyMCE skins to the public directory ...
Loading production environment (Rails 6.1.7.3)
irb(main):001:0> User.search("cyrpub")
=> 
[#<User id: 69172, firstname: "n.n.", surname: "n.n.", email: "cyrpub8@bollu.be", user_type_id: nil, user_status_id: nil, created_at: "2023-10-02 13:20:11.672392000 +0000", updated_at: "2023-10-02 13:20:11.672392000 +0000", dmponline3: nil, accept_terms: nil, org_id: 181, other_organisation: nil, api_token: nil, language_id: 1, recovery_email: nil, active: true, department_id: nil, last_api_access: nil, prefs: nil>,
 #<User id: 69171, firstname: "n.n.", surname: "n.n.", email: "cyrpub@bollu.be", user_type_id: nil, user_status_id: nil, created_at: "2023-10-02 10:36:01.259820000 +0000", updated_at: "2023-10-02 10:36:01.259820000 +0000", dmponline3: nil, accept_terms: nil, org_id: 181, other_organisation: nil, api_token: nil, language_id: 1, recovery_email: nil, active: true, department_id: nil, last_api_access: nil, prefs: nil>]
irb(main):002:0> User.invite!(email: "cyrpub7@bollu.be")
=> #<User id: 69173, firstname: nil, surname: nil, email: "cyrpub7@bollu.be", user_type_id: nil, user_status_id: nil, created_at: "2023-10-02 13:33:49.175679000 +0000", updated_at: "2023-10-02 13:43:28.310670984 +0000", dmponline3: nil, accept_terms: nil, org_id: nil, other_organisation: nil, api_token: nil, language_id: 1, recovery_email: nil, active: true, department_id: nil, last_api_access: nil, prefs: {}>
irb(main):003:0> 
root@roadmap-test:/opt/roadmap#

Here's the reasult:

root@dmponline:/opt/roadmap# docker inspect --format "{{ .State.Pid }}" "roadmap-test"
85884
(reverse-i-search)`nsse': rm -rf d^Csec-dmponline
root@dmponline:/opt/roadmap# nsenter -n -t 85884
root@dmponline:/opt/roadmap# tcpdump -i any
tcpdump: data link type LINUX_SLL2
tcpdump: verbose output suppressed, use -v[v]... for full protocol decode
listening on any, link-type LINUX_SLL2 (Linux cooked v2), snapshot length 262144 bytes
15:43:28.167383 eth0  Out IP 172.18.0.5.40070 > 172.18.0.3.5000: Flags [P.], seq 862874163:862874207, ack 133023267, win 582, options [nop,nop,TS val 529424682 ecr 2400271083], length 44
15:43:28.168548 eth0  In  IP 172.18.0.3.5000 > 172.18.0.5.40070: Flags [P.], seq 1:102, ack 44, win 501, options [nop,nop,TS val 2400308706 ecr 529424682], length 101
15:43:28.168574 eth0  Out IP 172.18.0.5.40070 > 172.18.0.3.5000: Flags [.], ack 102, win 582, options [nop,nop,TS val 529424683 ecr 2400308706], length 0
15:43:28.168933 eth0  Out IP 172.18.0.5.40070 > 172.18.0.3.5000: Flags [P.], seq 44:173, ack 102, win 582, options [nop,nop,TS val 529424683 ecr 2400308706], length 129
15:43:28.169859 eth0  In  IP 172.18.0.3.5000 > 172.18.0.5.40070: Flags [P.], seq 102:135, ack 173, win 501, options [nop,nop,TS val 2400308707 ecr 529424683], length 33
15:43:28.170101 eth0  Out IP 172.18.0.5.40070 > 172.18.0.3.5000: Flags [P.], seq 173:263, ack 135, win 582, options [nop,nop,TS val 529424685 ecr 2400308707], length 90
15:43:28.170952 eth0  In  IP 172.18.0.3.5000 > 172.18.0.5.40070: Flags [P.], seq 135:1755, ack 263, win 501, options [nop,nop,TS val 2400308708 ecr 529424685], length 1620
15:43:28.170985 eth0  Out IP 172.18.0.5.40070 > 172.18.0.3.5000: Flags [.], ack 1755, win 608, options [nop,nop,TS val 529424685 ecr 2400308708], length 0
15:43:28.220039 lo    In  IP localhost.39295 > 127.0.0.53.domain: 35080+ [1au] PTR? 3.0.18.172.in-addr.arpa. (52)
15:43:28.220075 lo    In  IP 127.0.0.53 > localhost: ICMP 127.0.0.53 udp port domain unreachable, length 88
15:43:28.220143 lo    In  IP localhost.36676 > 127.0.0.53.domain: 35080+ [1au] PTR? 3.0.18.172.in-addr.arpa. (52)
15:43:28.220155 lo    In  IP 127.0.0.53 > localhost: ICMP 127.0.0.53 udp port domain unreachable, length 88
15:43:28.220311 lo    In  IP localhost.42166 > 127.0.0.53.domain: 62524+ [1au] PTR? 5.0.18.172.in-addr.arpa. (52)
15:43:28.220324 lo    In  IP 127.0.0.53 > localhost: ICMP 127.0.0.53 udp port domain unreachable, length 88
15:43:28.220380 lo    In  IP localhost.43161 > 127.0.0.53.domain: 62524+ [1au] PTR? 5.0.18.172.in-addr.arpa. (52)
15:43:28.220391 lo    In  IP 127.0.0.53 > localhost: ICMP 127.0.0.53 udp port domain unreachable, length 88
15:43:28.298185 eth0  Out IP 172.18.0.5.40070 > 172.18.0.3.5000: Flags [P.], seq 263:403, ack 1755, win 608, options [nop,nop,TS val 529424813 ecr 2400308708], length 140
15:43:28.299264 eth0  In  IP 172.18.0.3.5000 > 172.18.0.5.40070: Flags [P.], seq 1755:1788, ack 403, win 501, options [nop,nop,TS val 2400308837 ecr 529424813], length 33
15:43:28.300996 eth0  Out IP 172.18.0.5.40070 > 172.18.0.3.5000: Flags [P.], seq 403:541, ack 1788, win 608, options [nop,nop,TS val 529424815 ecr 2400308837], length 138
15:43:28.303558 eth0  In  IP 172.18.0.3.5000 > 172.18.0.5.40070: Flags [P.], seq 1788:3022, ack 541, win 501, options [nop,nop,TS val 2400308841 ecr 529424815], length 1234
15:43:28.315255 eth0  Out IP 172.18.0.5.40070 > 172.18.0.3.5000: Flags [P.], seq 541:574, ack 3022, win 630, options [nop,nop,TS val 529424830 ecr 2400308841], length 33
15:43:28.316307 eth0  In  IP 172.18.0.3.5000 > 172.18.0.5.40070: Flags [P.], seq 3022:3061, ack 574, win 501, options [nop,nop,TS val 2400308854 ecr 529424830], length 39
15:43:28.317163 eth0  Out IP 172.18.0.5.40070 > 172.18.0.3.5000: Flags [P.], seq 574:918, ack 3061, win 630, options [nop,nop,TS val 529424832 ecr 2400308854], length 344
15:43:28.318536 eth0  In  IP 172.18.0.3.5000 > 172.18.0.5.40070: Flags [P.], seq 3061:3118, ack 918, win 501, options [nop,nop,TS val 2400308856 ecr 529424832], length 57
15:43:28.323545 lo    In  IP localhost.38302 > 127.0.0.53.domain: 3916+ [1au] PTR? 53.0.0.127.in-addr.arpa. (52)
15:43:28.323569 lo    In  IP 127.0.0.53 > localhost: ICMP 127.0.0.53 udp port domain unreachable, length 88
15:43:28.323636 lo    In  IP localhost.36477 > 127.0.0.53.domain: 3916+ [1au] PTR? 53.0.0.127.in-addr.arpa. (52)
15:43:28.323648 lo    In  IP 127.0.0.53 > localhost: ICMP 127.0.0.53 udp port domain unreachable, length 88
15:43:28.325863 eth0  Out IP 172.18.0.5.40070 > 172.18.0.3.5000: Flags [P.], seq 918:1122, ack 3118, win 630, options [nop,nop,TS val 529424840 ecr 2400308856], length 204
15:43:28.327216 eth0  In  IP 172.18.0.3.5000 > 172.18.0.5.40070: Flags [P.], seq 3118:3175, ack 1122, win 501, options [nop,nop,TS val 2400308865 ecr 529424840], length 57
15:43:28.327741 eth0  Out IP 172.18.0.5.40070 > 172.18.0.3.5000: Flags [P.], seq 1122:1156, ack 3175, win 630, options [nop,nop,TS val 529424842 ecr 2400308865], length 34
15:43:28.328980 eth0  In  IP 172.18.0.3.5000 > 172.18.0.5.40070: Flags [P.], seq 3175:3215, ack 1156, win 501, options [nop,nop,TS val 2400308866 ecr 529424842], length 40
15:43:28.371109 eth0  Out IP 172.18.0.5.40070 > 172.18.0.3.5000: Flags [.], ack 3215, win 630, options [nop,nop,TS val 529424886 ecr 2400308866], length 0
^C
33 packets captured
45 packets received by filter
0 packets dropped by kernel
root@dmponline:/opt/roadmap#
StCyr commented 9 months ago

I've tried to add export DMP_SMTP_PORT=34321

no change... :-/

nicolasfranck commented 9 months ago

Mm, maybe it has something to do with these lines:

That uses the function deliver_if that thus only delivers an email under certain conditions. I believe, in this that an email is only sent for owners and coowners.

nicolasfranck commented 9 months ago

But as you can see, that applies to the current production code too.

nicolasfranck commented 9 months ago

and more: that function fetches the recipient from the user table, and looks up that users want to receive emails:

Rails console:

> user = User.find_by_email("nicolas.franck@ugent.be")
> user.get_preferences('email')
{:users=>
  {:new_comment=>false,
   :admin_privileges=>true,
   :added_as_coowner=>true,
   :feedback_requested=>true,
   :feedback_provided=>true},
 :owners_and_coowners=>{:visibility_changed=>true}}

So that is where that key in deliver_if comes from. Mm, if you update your email settings in your profile, then all will be fine?

nicolasfranck commented 9 months ago

btw invites for newly created users are always sent.

StCyr commented 9 months ago

My issue seems more simple than that:

In the code:

         # rubocop:disable Metrics/BlockNesting
          if user.nil?
            registered = false
            User.invite!({ email: role_params[:user][:email],
                           firstname: _('First Name'),
                           surname: _('Surname'),
                           org: current_user.org },
                         current_user)
            message = format(_('Invitation to %{email} issued successfully.'),
                             email: role_params[:user][:email])
            user = User.where_case_insensitive('email', role_params[:user][:email]).first
          end

          message += format(_('Plan shared with %{email}.'), email: user.email)
          @role.user = user

          if @role.save
            if registered
              deliver_if(recipients: user, key: 'users.added_as_coowner') do |r|
                UserMailer.sharing_notification(@role, r, inviter: current_user)
                          .deliver_now
              end
            end
            flash[:notice] = message

we go through the first if (if user.nil?), so the register variable is set to false. Thus, deliver_if is not even called.

So, it seems that's just that notifications are not sent to unregistered users "by design"?!?

nicolasfranck commented 9 months ago

Yes, it seems so

The invite mail is not sent for some reason.

That variable registered remains false. So the second mail, that informs the user about the plan is not sent.

That makes sense for dmproadmap, where users are never automatically registered. Normally unconfirmed users like that are "blocked" by user interface, but I have overridden that: when a users arrives from ORCID or Shibboleth, then that user record is unlocked.

Will investigate

nicolasfranck commented 9 months ago

Found this also: we disable the invitation mail:

https://github.com/DMPbelgium/roadmap/blob/master/config/initializers/ugent.rb#L984

Crap, I remember, in the past, both mails were sent: both the invitation mail and the share notification mail. So I disabled the invitation mail, so only the sharing notification mail would be received.

But that seems like a long long time ago (DMPonline_v4?). I see that in their rework in 2017, they had the same idea:

https://github.com/dmproadmap/roadmap/issues/307

but with a different solution: send the invitation mail, but not the sharing notification email.

For you information, this is the invitation email:

Hello n.n. n.n.

Your colleague nicolas.franck@ugent.be has invited you to contribute to their Data Management Plan in DMPonline.be

[Click here](http://localhost:3000/users/invitation/accept?invitation_token=zf6jmEt7kMPSVER-T_6N) to accept the invitation, (or copy http://localhost:3000/users/invitation/accept?invitation_token=zf6jmEt7kMPSVER-T_6N into your browser). If you don't want to accept the invitation, please ignore this email.

All the best
The DMPonline.be team

Please do not reply to this email.  If you have any questions or need help, please contact us at or visit http://localhost:3000/contact-us

See also https://github.com/DMPbelgium/roadmap/blob/master/app/views/devise/mailer/invitation_instructions.html.erb

I can reenable the invitation of course, but we'll have to come up with some alternate text that does not mention the invitation token. And we do not have access to variable "plan" like in the sharing notification email.

nicolasfranck commented 9 months ago

@StCyr see https://github.com/DMPbelgium/roadmap/commit/e8118bf0a71718343c0b7914f8c5d45d7c8dbb6a for fix.

I basically reenabled the invitation mail, and made it look like the regular sharing notification mail. The only difference is that it links to the full list of plans (/plans) instead of to the specific plan, which I cannot determine at that exact moment the mail is generated.

StCyr commented 9 months ago

applied on test.dmponline.be.

seems working good :-)

TheLisaVL commented 9 months ago

Tested with all permission types: co-owner, editor & read only. Mail sent and received in all cases

TheLisaVL commented 4 months ago

Fixed in the december '23 update