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

Subsribe and unsubscribe functionality #3035

Closed
wants to merge 11 commits into from
38 changes: 23 additions & 15 deletions src/auth-service/config/global/email-templates.js
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this @BenjaminSsempala ,

  • Could you share some background notes to this variable declaration? I though environment specific variables are handled under config/environments. Could you shed more light on this?
  • Ideally, the baseURL is supposed to be determined at runtime of the application --- so we might want to just simply call constants.PLATFORM_BASE_URL or constants.ANALYTICS_BASE_URL. See attached screenshot for visual reference.
  • On a side note, we shall also need to be mindful of cyclic dependencies as we go about this.
    Screenshot 2024-04-09 at 07 58 56

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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>
Expand All @@ -23,7 +25,23 @@
</table>
`;
},
EMAIL_FOOTER_TEMPLATE: function (email) {
EMAIL_FOOTER_TEMPLATE: function (email, type, paramString) {
let subscriptionBlock = ``;

Check warning on line 29 in src/auth-service/config/global/email-templates.js

View check run for this annotation

Codecov / codecov/patch

src/auth-service/config/global/email-templates.js#L28-L29

Added lines #L28 - L29 were not covered by tests
if (type && paramString) {
const unSubsciptionUrl = `${baseUrl}/api/v2/users/unsubscribe/${type}?${paramString}`;
subscriptionBlock = `

Check warning on line 32 in src/auth-service/config/global/email-templates.js

View check run for this annotation

Codecov / codecov/patch

src/auth-service/config/global/email-templates.js#L31-L32

Added lines #L31 - L32 were not covered by tests
<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>

Check warning on line 37 in src/auth-service/config/global/email-templates.js

View check run for this annotation

Codecov / codecov/patch

src/auth-service/config/global/email-templates.js#L37

Added line #L37 was not covered by tests
</a>
<span
style="color: #667085; font-size: 14px; font-family: Inter; font-weight: 400; line-height: 20px; word-wrap: break-word;"></span>
<br /><br />
`;

Check warning on line 42 in src/auth-service/config/global/email-templates.js

View check run for this annotation

Codecov / codecov/patch

src/auth-service/config/global/email-templates.js#L41-L42

Added lines #L41 - L42 were not covered by tests
BenjaminSsempala marked this conversation as resolved.
Show resolved Hide resolved
}

BenjaminSsempala marked this conversation as resolved.
Show resolved Hide resolved
return `
<table style="width: 100%; text-align: center; padding-top: 32px; padding-bottom: 32px;">
<tr>
Expand Down Expand Up @@ -53,17 +71,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 />
Expand All @@ -76,11 +84,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);

Check warning on line 88 in src/auth-service/config/global/email-templates.js

View check run for this annotation

Codecov / codecov/patch

src/auth-service/config/global/email-templates.js#L87-L88

Added lines #L87 - L88 were not covered by tests
const headerTemplate = this.EMAIL_HEADER_TEMPLATE();
let greetings = this.EMAIL_GREETINGS(name);
if (!name) {
if (!name || name === "") {
Comment on lines +86 to +90
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Please add test coverage for the updated EMAIL_BODY function.

The changes to the EMAIL_BODY function look good and ensure that the unsubscribe link is included in the email body when applicable.

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 EMAIL_BODY function is adequately tested to prevent future regressions.

Do you want me to help add test cases specifically for the updated EMAIL_BODY function? I can provide guidance or even generate the test code for you.

Tools
GitHub Check: codecov/patch

[warning] 86-87: src/auth-service/config/global/email-templates.js#L86-L87
Added lines #L86 - L87 were not covered by tests

greetings = ``;
}
return `<!DOCTYPE html>
Expand Down
61 changes: 40 additions & 21 deletions src/auth-service/models/Subscription.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -44,7 +50,7 @@ const SubscriptionSchema = new mongoose.Schema(
type: Boolean,
default: true,
},
},
}
},
{
timestamps: true,
Expand All @@ -63,9 +69,10 @@ SubscriptionSchema.methods = {
toJSON() {
return {
_id: this._id,
email: this.subscribed,
isSystemUser: this.isSystemUser,
email: this.email,
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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,
};
},
};
Expand Down Expand Up @@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

The 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 (
Expand All @@ -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(
Expand Down
6 changes: 0 additions & 6 deletions src/auth-service/models/User.js
Original file line number Diff line number Diff line change
Expand Up @@ -195,12 +195,6 @@ const UserSchema = new Schema(
category: {
type: String,
},
notifications: {
email: { type: Boolean, default: false },
push: { type: Boolean, default: false },
text: { type: Boolean, default: false },
phone: { type: Boolean, default: false },
},
profilePicture: {
type: String,
},
Expand Down
Loading
Loading