-
Notifications
You must be signed in to change notification settings - Fork 22
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
Subsribe and unsubscribe functionality #3035
Changes from all commits
f438919
f27d5c8
97c19ab
0093a38
301bc85
6f4e083
628cccf
973c8ba
9473b72
6f81753
9c6a76d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,8 @@ | ||
const mongoose = require("mongoose"); | ||
const ObjectId = mongoose.Types.ObjectId; | ||
|
||
const baseUrl = process.env.PLATFORM_STAGING_BASE_URL || process.env.PLATFORM_PRODUCTION_BASE_URL; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using a configuration service for defining URLs. The use of environment variables directly in the code can lead to hard-coded configurations that are difficult to manage across different environments. Consider using a centralized configuration service or module that can handle these URLs, which can be easily adjusted without changing the codebase. |
||
|
||
Comment on lines
+4
to
+5
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using a configuration service for defining URLs. The use of environment variables directly in the code can lead to hard-coded configurations that are difficult to manage across different environments. Consider using a centralized configuration service or module that can handle these URLs, which can be easily adjusted without changing the codebase. |
||
const emailTemplates = { | ||
EMAIL_GREETINGS: function (name) { | ||
return `<tr> | ||
|
@@ -23,7 +25,22 @@ | |
</table> | ||
`; | ||
}, | ||
EMAIL_FOOTER_TEMPLATE: function (email) { | ||
EMAIL_FOOTER_TEMPLATE: (email, type, paramString) => { | ||
let subscriptionBlock = ``; | ||
if (type && paramString) { | ||
const unSubsciptionUrl = `${baseUrl}/api/v2/users/unsubscribe/${type}?${paramString}`; | ||
subscriptionBlock = ` | ||
<span | ||
style="color: #667085; font-size: 14px; font-family: Inter; font-weight: 400; line-height: 20px; word-wrap: break-word;">. | ||
If you'd rather not receive this kind of email, you can </span> | ||
<a href=${unSubsciptionUrl} target="_blank" <span | ||
style="color: #135DFF; font-size: 14px; font-family: Inter; font-weight: 400; line-height: 20px; word-wrap: break-word;">unsubscribe!</span> | ||
</a> | ||
<span | ||
style="color: #667085; font-size: 14px; font-family: Inter; font-weight: 400; line-height: 20px; word-wrap: break-word;"></span> | ||
<br /><br /> | ||
`; | ||
} | ||
Comment on lines
+28
to
+43
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LGTM! Please add test coverage for the dynamic URL generation. The changes to the However, as indicated by the static analysis hints, there is missing test coverage for the added lines. It's important to ensure that the dynamic URL generation is adequately tested to prevent future regressions. Do you want me to help add test cases specifically for the dynamic URL generation in ToolsGitHub Check: codecov/patch
|
||
return ` | ||
<table style="width: 100%; text-align: center; padding-top: 32px; padding-bottom: 32px;"> | ||
<tr> | ||
|
@@ -53,17 +70,7 @@ | |
email was sent to</span> | ||
<span | ||
style="color: #135DFF; font-size: 14px; font-family: Inter; font-weight: 400; line-height: 20px; word-wrap: break-word;">${email}</span> | ||
<span | ||
style="color: #667085; font-size: 14px; font-family: Inter; font-weight: 400; line-height: 20px; word-wrap: break-word;">. | ||
If you'd rather not receive this kind of email, you can </span> | ||
<span | ||
style="color: #135DFF; font-size: 14px; font-family: Inter; font-weight: 400; line-height: 20px; word-wrap: break-word;">unsubscribe</span> | ||
<span | ||
style="color: #667085; font-size: 14px; font-family: Inter; font-weight: 400; line-height: 20px; word-wrap: break-word;"> | ||
or </span> | ||
<span | ||
style="color: #135DFF; font-size: 14px; font-family: Inter; font-weight: 400; line-height: 20px; word-wrap: break-word;">manage | ||
your email preferences.</span><br /><br /> | ||
${subscriptionBlock} | ||
<span | ||
style="color: #667085; font-size: 14px; font-family: Inter; font-weight: 400; line-height: 20px; word-wrap: break-word;">© | ||
2023 AirQo<br /><br /> | ||
|
@@ -76,11 +83,11 @@ | |
`; | ||
}, | ||
|
||
EMAIL_BODY: function (email, content, name) { | ||
const footerTemplate = this.EMAIL_FOOTER_TEMPLATE(email); | ||
EMAIL_BODY: function (email, content, name, type, paramString) { | ||
const footerTemplate = this.EMAIL_FOOTER_TEMPLATE(email, type, paramString); | ||
const headerTemplate = this.EMAIL_HEADER_TEMPLATE(); | ||
let greetings = this.EMAIL_GREETINGS(name); | ||
if (!name) { | ||
if (!name || name === "") { | ||
Comment on lines
+86
to
+90
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LGTM! Please add test coverage for the updated The changes to the However, as indicated by the static analysis hints, there is missing test coverage for the added lines. It's important to ensure that the updated Do you want me to help add test cases specifically for the updated ToolsGitHub Check: codecov/patch
|
||
greetings = ``; | ||
} | ||
return `<!DOCTYPE html> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,10 +32,16 @@ const SubscriptionSchema = new mongoose.Schema( | |
type: Boolean, | ||
default: true, | ||
}, | ||
|
||
email: { | ||
type: Boolean, | ||
default: true, | ||
}, | ||
|
||
mobile_push: { | ||
type: Boolean, | ||
default: true, | ||
}, | ||
phone: { | ||
type: Boolean, | ||
default: true, | ||
|
@@ -44,7 +50,7 @@ const SubscriptionSchema = new mongoose.Schema( | |
type: Boolean, | ||
default: true, | ||
}, | ||
}, | ||
} | ||
}, | ||
{ | ||
timestamps: true, | ||
|
@@ -63,9 +69,10 @@ SubscriptionSchema.methods = { | |
toJSON() { | ||
return { | ||
_id: this._id, | ||
email: this.subscribed, | ||
isSystemUser: this.isSystemUser, | ||
email: this.email, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @BenjaminSsempala , this is great progress. Awesome! I am envisioning a future where we try our level best NOT to categorise users but rather, just approach it as AirQo users. In summary, the current status of things should not have a direct influence on our implementations. I am more than happy to sync further on the strategy. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, @Baalmart We could have a sync to review the strategy. But to offer more light, the categories were more for the products, i.e., app notifications and analytics notifications. |
||
notifications: this.notifications, | ||
subscribed: this.subscribed, | ||
isSystemUser: this.isSystemUser, | ||
}; | ||
}, | ||
}; | ||
|
@@ -250,8 +257,24 @@ SubscriptionSchema.statics.remove = async function ( | |
} | ||
}; | ||
|
||
SubscriptionSchema.statics.subscribe = async function (email, type) { | ||
const updatedSubscription = await this.findOneAndUpdate( | ||
{ email }, | ||
{ $set: { [`notifications.${type}`]: true } }, | ||
{ new: true, upsert: true } | ||
); | ||
|
||
return updatedSubscription; | ||
}; | ||
|
||
SubscriptionSchema.statics.unsubscribe = async function (email, type) { | ||
await this.updateOne({ email }, { [`notifications.${type}`]: false }); | ||
const updatedSubscription = await this.findOneAndUpdate( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great work here, thanks @BenjaminSsempala |
||
{ email }, | ||
{ $set: { [`notifications.${type}`]: false } }, | ||
{ new: true, upsert: true } | ||
); | ||
|
||
return updatedSubscription; | ||
}; | ||
|
||
SubscriptionSchema.statics.checkNotificationStatus = async function ( | ||
|
@@ -270,31 +293,27 @@ SubscriptionSchema.statics.checkNotificationStatus = async function ( | |
message: `No subscription found for email: ${email}`, | ||
}, | ||
}; | ||
} else if (isEmpty(subscription.notifications[type])) { | ||
return { | ||
success: true, | ||
message: `not subscribed to type`, | ||
status: httpStatus.OK, | ||
errors: { | ||
message: `User is not subscribed to ${type} notifications`, | ||
}, | ||
}; | ||
} else if (subscription.notifications[type] === false) { | ||
} | ||
|
||
let isSubscribed = subscription.notifications[type]; | ||
|
||
|
||
if (!isSubscribed) { | ||
return { | ||
success: false, | ||
message: `Forbidden`, | ||
status: httpStatus.FORBIDDEN, | ||
errors: { | ||
message: `User unsubscribed from ${type} notifications`, | ||
message: `User not subscribed to ${type} notifications.`, | ||
}, | ||
}; | ||
} else { | ||
return { | ||
success: true, | ||
message: `User is subscribed to ${type} notifications`, | ||
status: httpStatus.OK, | ||
}; | ||
} | ||
|
||
return { | ||
success: true, | ||
message: `User is subscribed to ${type} notifications.`, | ||
status: httpStatus.OK, | ||
}; | ||
} catch (error) { | ||
logger.error(`Data conflicts detected -- ${error.message}`); | ||
next( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this @BenjaminSsempala ,
environment
specific variables are handled underconfig/environments
. Could you shed more light on this?constants.PLATFORM_BASE_URL
orconstants.ANALYTICS_BASE_URL
. See attached screenshot for visual reference.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Baalmart , Yes, I failed to use the constants variable because of circular dependencies.