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

Send An email to new user is not working #2424

Closed
6 of 8 tasks
Icidis opened this issue Oct 24, 2018 · 27 comments · Fixed by #2492
Closed
6 of 8 tasks

Send An email to new user is not working #2424

Icidis opened this issue Oct 24, 2018 · 27 comments · Fixed by #2492

Comments

@Icidis
Copy link

Icidis commented Oct 24, 2018

Description of bug

When creating a new user account as a SuperUser/Admin and checking the "Send An Email To New User." checkbox, no email is sent to the email I specify.

Once the user is created, sending password reset links, etc. work fine. Emails are sent and received correctly so it appears the SMTP settings are correct.

Steps to reproduce

List the steps to reproduce the behavior:

  1. Go to 'Manage Users'
  2. Click on 'Add User'
  3. Fill in user details
  4. Check the "Send An Email To New User" checkbox
  5. Click "Save"

Current result

The user is created successfully but no email is send to the user.

Expected result

User gets created and registration email is sent to the users' email address.

Additional context

SMTP is configured to use SendGrid smtp but password reset etc do work without any issues.

Affected version

  • 9.2.2
  • 9.2.1
  • 9.2

Affected browser

  • Chrome
  • Firefox
  • Safari
  • Internet Explorer
  • Edge
@sleupold
Copy link
Contributor

Which registration type ate you using - public, verified or private?

@Icidis
Copy link
Author

Icidis commented Oct 24, 2018

Hi Sebastian (@sleupold),

The "User Registration" setting is set to "None"

@Tychodewaard
Copy link
Contributor

I have a customer who has 'verified' as user registration with the same issue.

@sleupold
Copy link
Contributor

AFAIR there has been already a PR merged in this area, I am not sure, whether it fixes this issue as well. It would be great if anyone could test it in a nightly build.

@Icidis
Copy link
Author

Icidis commented Oct 24, 2018

I've upgraded the site from version 9.2.2 to the Nightly Build 9.3.0-alpha.31 and the same issue persists.

@jacques-hoventer
Copy link

Is there any known workaround for this?

@mikebigun
Copy link
Contributor

Dear community,

Email notification is not handled for the User Registration None type. Initially, if None is set, we shouldn't allow anyone to create users.
Nevertheless, I found property IgnoreRegistrationMode which overwrites above logic and makes us ALWAYS possible to create new users. See below:

https://github.com/dnnsoftware/Dnn.AdminExperience/blob/95513df2b8440bc0d37316545d6dd327a3f30506/Extensions/Manage/Dnn.PersonaBar.Users/Components/Contracts/RegisterationDetails.cs#L41

This property was introduced within https://dnntracker.atlassian.net/browse/SOCIAL-3158 Unfortunately, do not have access to check what is it about.

The default value of this property is hardcoded and is true. It is not cumming from UI so it is not configurable. With this property, it is senseless to use None registration mode.

https://github.com/dnnsoftware/Dnn.AdminExperience/blob/95513df2b8440bc0d37316545d6dd327a3f30506/Extensions/Manage/Dnn.PersonaBar.Users/Services/UsersController.cs#L81

To fix this issue we should either disallow user registration when mode is None (so the issue will gone) or add missing functionality to send welcome email for the newly created user.

Please, share your thoughts.

@sleupold
Copy link
Contributor

sleupold commented Nov 26, 2018

@mikebigun,
registration type "None" does not mean, no-one can create a user account, but there is no "register" option for self registration on the site. Superusers and Site Admins are always allowed to manage site accounts. This is DNN behavior since DNN 1 and should not be changed IMO.
I have no clue, what DNN Social is overriding, maybe an integration with another event like buying a product, which offers to create (or auto-creates) a user account for the product social site (just a guess)

@mikebigun
Copy link
Contributor

@sleupold thanks for explanation.
Just confused a bit. When admin is creating new user, UserController -> CreateUser->RegisterController.Instance.Register is invoked.
Inside Register there is list of registration types for which email is going to be sent.

https://github.com/dnnsoftware/Dnn.AdminExperience/blob/95513df2b8440bc0d37316545d6dd327a3f30506/Extensions/Manage/Dnn.PersonaBar.Users/Components/RegisterController.cs#L272

So Private/Public/Verified types are handled. None is not there, so email sending is ignored.

@sleupold
Copy link
Contributor

@mikebigun ,
I'd like to describe, what should happen:

  • if a user clicks register and registration is set to "public", he is granted access immediately and gets an email sent with site name, URL and login for reference. The site admin(s) might get a notification about the new user.
  • if a user clicks register and registration is set to "verified", the user gets limited access (unverified user) and an email with a confirmation link, he needs to click to gain full access. the site admin(s) gets a notification about the new user.
  • if a user clicks register and registration is set to "private", the user gets an email confirming his registration and the info, the admin has been notified. The site admin(s) gets a notification about the user awaiting registration
  • if an admin creates a user account (any registration mode), the user gets an email about his new account with site name, site URL, user name and an option to obtain his password.

@sleupold
Copy link
Contributor

@mikebigun
I don't know the reason, maybe the code for PB has been inspired by wrong source code part of DNN 8 - just a guess.

@jacques-hoventer
Copy link

I agree with @sleupold fourth point which is not happening at the moment. One thing I would add though is that when an administrator creates a user it should be a choice whether or not to send the user an email, but the default should be to send the user an email.

@Icidis
Copy link
Author

Icidis commented Nov 27, 2018

@jacques-hoventer There's a checkbox already which the administrator can check/un-check to indicate that an email gets sent to the user or not.

@jacques-hoventer
Copy link

@Icidis to my knowledge that checkbox is ignored if your portal is set to Registration > None, which shouldn't be the case. PS. This is in Dnn 9.2.2

@Icidis
Copy link
Author

Icidis commented Nov 27, 2018

@jacques-hoventer I fully agree, that's why I reported the issue. I'm trying to pinpoint the exact issue at the moment.

@jacques-hoventer
Copy link

@Icidis apologies, I reread your post and realized that's what it was about.

@Icidis
Copy link
Author

Icidis commented Nov 27, 2018

After reviewing the flow of the user creation from user admin. I found the following.

There is an event "UserCreated" which could be updated to include the below code

Mail.SendMail(args.User, MessageType.UserRegistrationPublic, PortalSettings.Current);

but of course you don't know which template/message type to use when sending the email.

public void UserCreated(object sender, UserEventArgs args)

Better solution:
I would say the code change should be done here:

https://github.com/dnnsoftware/Dnn.AdminExperience/blob/95513df2b8440bc0d37316545d6dd327a3f30506/Extensions/Manage/Dnn.PersonaBar.Users/Components/RegisterController.cs#L272

as described by @mikebigun .

I'll create an issue and pull request on that repo for review.

@sleupold
Copy link
Contributor

IMO we should fix it in the API, not in the UI, which is not accessed by 3rd party extensions.
We may need to extend the method with additional parameters, if necessary.

@Icidis
Copy link
Author

Icidis commented Nov 27, 2018

Changing it in the API would be the best solution. But I would imagine that would take some time to discuss with all the relevant parties including the changes to the API

I would be happy with the change in the UI now as the email notifications sit there already and then later change to a more appropriate solution.

@mikebigun
Copy link
Contributor

I would appreciate it if there is a chance to fix it in the current implementation. The issue reported exactly for the persona bar and considered as a bug. It is expected to be fixed asap.
Moving to API changes will require long discussion involving all API consumers. I agree, this is certainly right direction for the further improvements, still we need to make correction for the existing part.

bdukes pushed a commit that referenced this issue Dec 19, 2018
This is alternative way to fix above issue proposed in dnnsoftware/Dnn.AdminExperience#174

As per @sleupold , we need to move email notifications from UI to core part.
Once this will be approved and merged, we can remove email notifications from UI and replace it with updated controller method to let notifications to be send to their recipients.

fixes #2424
@Icidis
Copy link
Author

Icidis commented Jan 23, 2019

Can we re-open this issue?

Just tested using the 9.3.0 9018 build as per below.

https://dotnet.visualstudio.com/DNN/_build/results?buildId=9018

The code changed still relies on the PersonaBar to pass in the sendEmailNotification.

Here seems to be the flow:

PersonaBar (Dnn.AdminExperience)

  1. Some Javascript calls this function - RegisterController.Instance.Register(settings);
  2. RegisterController.Register Shouldn't this line be var createStatus = UserController.CreateUser(ref newUser, registerationDetails.Notify);

DNN.Platform

  1. UserController.CreateUser - sendEmailNotification parameter is false
  2. UserController.CreateUser - EventManager.Instance.OnUserCreated is called.
  3. UserEventHandlers.UserCreated Gets called and the UserEventArgs.SendNotification = false

After all this, the new UserRegistrationEmailNotifier.NotifyUser is never called. Am I missing something, can anyone reproduce this?

Also why is the PersonaBar (Dnn.AdminExperience) still doing the SendMail when we have this new service to handle all of this?

@Icidis
Copy link
Author

Icidis commented Jan 23, 2019

@sleupold @bdukes @mikebigun Do I need to open a new issue or by adding a new comment re-opens this issue?

@sleupold
Copy link
Contributor

@bdukes
IMO it should be re-opened

@valadas
Copy link
Contributor

valadas commented Jan 23, 2019

I need to review this in more details later but I think what happened was:

PR A was submitted to AdminExperience but the comments suggested it would be better to have the core API updated instead and to add a PR B in AdminExperience to use that the core API.

Then came PR C to fix the core API and was merged with a comment that there would need to be another PR D for the AdminExperience to use the new API from PR C.

PR A was merged but according to the plan of having the PR D coming, it would have sent the email twice. So it was reverted. The issue I thing is that PR D never came.

Sorry for the letters I am on mobile now. Will update later with actual PR numbers

@valadas
Copy link
Contributor

valadas commented Jan 25, 2019

Ok, so my review was correct, the UI was just not updated to reflect the changes on #2492

As per https://www.dnnsoftware.com/wiki/registration-types/page the behaviour of none is correct, users cannot register themselves but admins can, and they should be able to notify the user if they so want. So I opened dnnsoftware/Dnn.AdminExperience#363 to handle that second part of the fix. Will be submitting a PR for that shortly.

zyhfish pushed a commit to zyhfish/Dnn.Platform that referenced this issue Mar 29, 2019
)

This is alternative way to fix above issue proposed in dnnsoftware/Dnn.AdminExperience#174

As per @sleupold , we need to move email notifications from UI to core part.
Once this will be approved and merged, we can remove email notifications from UI and replace it with updated controller method to let notifications to be send to their recipients.

fixes dnnsoftware#2424
mitchelsellers pushed a commit that referenced this issue May 14, 2019
* DNN-27517: force user logout after password changed in other place.

* DNN-27517: update code by review.

* DNN-27517: add host settings to control whether force logout after password changed.

* NOJIRA: mark as stable.

* Fixed bugs on add/remove user permissions for modules

* Change algorithm to SHA1CryptoServiceProvider

* Updated Issue Templates to include new RFC template and to support submissions for 9.3.0 release

* Corrected structure to avoid issue linking

* code review

* User registration: end the response after redirect (#2511)

* Initial New User Email Not Sending At Time of Creation (#2492)

This is alternative way to fix above issue proposed in dnnsoftware/Dnn.AdminExperience#174

As per @sleupold , we need to move email notifications from UI to core part.
Once this will be approved and merged, we can remove email notifications from UI and replace it with updated controller method to let notifications to be send to their recipients.

fixes #2424

* Fix for missing SQL change (#2522)

Fixes #2521 by rebuilding the PortalsDefaultLanguage view

* Resolve UserProfile Loading Errors From Unsecure pages (#2494)

* NOJIRA: mark as stable.

* DNN-21637: add config key.

* DNN-26576: prevent same-origin errors when loading popup and iframes from a secure page.

* code review

* Code review

* (DNN-10795) - All pages except home page return 404 (#2032)

* DNN-10795 - All pages except home page return 404

I have witnessed that occasionally on app pool recycle all,except the home page, will return 404 until the application pool is recycled a second time.

I've reviewed the code & believe that the root cause of the issue is due to the fact that the code that builds the tab index, portalDepths dictionary & tabPaths dictionary is not thread safe. I can see code in the method TabIndexController.FetchTabDictionary is using SharedDictionary classes to store the tab dictionaries, however the code is not thread safe when adding the dictionaries to the cache. Therefore when multiple threads are executing the FetchTabDictionary method it's possible for an empty dictionary to be added to the cache.

To resolve this issue the code has been updated so that only one thread can add the dictionaries to cache at a time.

* Updated comment to trigger Code Licence workflow.

* Added compiled DLLs that include the fix for bug DNN-10795 (All pages except home page return 404) for DNn versions 8.0.4 through 9.2.2

* Recursive read lock acquisitions not allowed (#2423)

* DNN-23293 Recursive read lock acquisitions not allowed in this mode.

* DNN-23293 Recursive read lock acquisitions not allowed in this mode.

* Performance problems when huge number of portal aliases is in use (#2514)

* DNN-27498 Performance Issues

* DNN-27498 Performance Issues

* minor formatting

* Fixed case sensitivity issue

* Added mixed cased alias support to unit tests

* Fixed VanityUrl unit tests

* Fixed broken LockStrategy unit tests (#2531)

* Delete Fixed-DLLs folder that was added as part of PR for bug DNN-10795. (#2535)

* Modules > ModuleCreator > fixed path error (#2527)

* Fixed issue in ModuleCreator > Web > template.ascx

* Update DNN Platform/Admin Modules/Dnn.Modules.ModuleCreator/Templates/Web/Module - HTML/template.ascx

Co-Authored-By: mean2me <emanuele.colonnelli@gmail.com>

* All languages are highlighted along with current
- add css for languages

* Log name of package when uninstalling extensions (#2557)

* remove spaces

* DNN-20856 After export with Content Localization site language flags disappears from pages (#2578)

* Fixed parallel build (#2562)

* Set active Nuget package source to All

* Fixed parallel build

* Inclusion of NDepend logo on the readme. (#2598)

* Fix for missing SQL change

Fixes #2521 by rebuilding the PortalsDefaultLanguage view

* Added attribution to NDepend for the usage of their ADO tooling

* Fix image/link markdown

* Get language from transferred parameter (#2607)

* switch encrypt method. (#2616)

* DNN-29484: switch encrypt method.

* NuGet Package Improvements

Changes to modernize the NuGet packages published by the DNN Platform, fixes #2586.  The below-submitted changes in structure have been validated by consultation with the DNN Platform Community, Microsoft Representatives, as well as validation of documentation per the published .nuspec file definition (https://docs.microsoft.com/en-us/nuget/reference/nuspec)

In detail, the following items have been changed:

* Migration of license information to the suggested <license> node rather than the deprecated <licenseurl> node.
* Inclusion of target framework for all included .dll files, this prevents installation of the package to pre-4.5 projects protecting downstream users.
* Improved package descriptions based on discussions held in the RFC regarding these improvements
* Added Package-to-Package dependencies to ensure quick usage and inclusion
* Updated the WebAPI and MVC packages to be holistic packages, including references to ALL needed items to develop using those patterns.

All changes are current for DNN Platform version 9.3.0 or later.  Packages have been built & tested locally with success.

## Suggested Usage

With these improved packages, development & references should be easier.

### MVC Modules

`Install-Package DotNetNuke.Web.Mvc`

Should be the only needed package installation.  It will install all needed dependencies, including the items necessary for WebAPI

### Modules Needing WebAPI (Not MVC)

`Install-Package DotNetNuke.WebApi`

Should be the only needed package for extensions not using MVC, however, needing to use WebApi for services.  This will work well for WebForms or Library projects, etc. that don't need the extra references for MVC/Razor

### WebForms/Limited Modules

`Install-Package DotNetNuke.Core`

The most simple modules, still using the WebForms pattern can use this package for the smallest footprint

For #2600

* Adjust the Source package to include changes from GitVersion (#2609)

* remove old ckeditor packaging steps

* Remove version to allow GitVersion to set it at build time (#2639)

* Adding 09.03.01.SqlDataProvider file

* Upgrade DNN to .NET Framework 4.7.2 (#2644)

* Upgraded app projects to .NET Framework 4.7.2; Added missing dependency to DotNetNuke.Tests.Core as it was missing DotNetNuke.Web.Client

* Removed targetframework web.config reference from Dnn.Modules.Console

* Reverted unintended changes
@markandcurry
Copy link

Hi has this issue been fixed?

@valadas
Copy link
Contributor

valadas commented Jul 10, 2019

As far as I can tell, this should be resolved in 9.3.1, if you can still reproduce after 9.3.1 @markandcurry please create a new issue with the version number affected. Thanks.

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 a pull request may close this issue.

7 participants