32678 - GOVM account can't receive email if no admin email set - fix#3682
32678 - GOVM account can't receive email if no admin email set - fix#3682Jxio wants to merge 2 commits intobcgov:mainfrom
Conversation
| if client.email: | ||
| email_list = ",".join( | ||
| {*(email_list.split(",") if email_list else []), client.email.strip()} | ||
| ) |
There was a problem hiding this comment.
probably should be moved to a function? I'm sure we use something like it somewhere else?
It's also not clear what this does I think it's converting to list, add new email and removing duplicates while stripping whitespaces?
Why not just use a function name for it?
eg. add_unique_email
| origin_url = current_app.config.get("WEB_APP_URL") | ||
| if admin_emails != "": | ||
| client: UserModel = UserModel.find_by_id(client_id) | ||
| client_login_source = client.login_source |
There was a problem hiding this comment.
why not just use
client.login_source
I don't really see you doing anything with client_login_source other than just passing it in
| email_list = UserService.get_admin_emails_for_org(org_id) | ||
| origin_url = current_app.config.get("WEB_APP_URL") | ||
| if admin_emails != "": | ||
| client: UserModel = UserModel.find_by_id(client_id) |
There was a problem hiding this comment.
client = UserModel.find_by_id(client_id)
you don't need the type it should be inferred
| if email_list != "": | ||
| if org.access_type in (AccessType.EXTRA_PROVINCIAL.value, AccessType.REGULAR_BCEID.value): | ||
| Org.send_approved_rejected_notification(admin_emails, org.name, org.id, org.status_code, client_id) | ||
| Org.send_approved_rejected_notification(email_list, org.name, org.id, org.status_code, client_login_source) |
There was a problem hiding this comment.
user.email Probably doesn't work for these? user.email was only intended for GOVM - BCEID I think should still come from contacts?
Sent you proof in teams
There was a problem hiding this comment.
updated: if org is GOVM, use user.email
| elif org.access_type in (AccessType.GOVM.value, AccessType.GOVN.value): | ||
| Org.send_approved_rejected_govm_govn_notification( | ||
| admin_emails, org.name, org.id, org.status_code, origin_url, client_id | ||
| email_list, org.name, org.id, org.status_code, origin_url, client_login_source |
There was a problem hiding this comment.
GOVN using user.email? it could be wrong? GOVN could be BCSC which would be contact.email
Sent you proof in teams
|
2 participants
Issue #:
bcgov/entity#32678
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the sbc-auth license (Apache 2.0).