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.

Sign up for GitHub

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
Closed

Subsribe and unsubscribe functionality #3035

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
f438919
Initial commit
BenjaminSsempala Feb 14, 2024
f27d5c8
Adding unsubscription Link
BenjaminSsempala Feb 17, 2024
97c19ab
Fine tuning Subscription Model
BenjaminSsempala Mar 27, 2024
0093a38
Merge branch 'staging' into email-subscribe
BenjaminSsempala Mar 27, 2024
301bc85
Adjusting the schema
BenjaminSsempala Apr 4, 2024
6f4e083
Removing " Product "
BenjaminSsempala Apr 8, 2024
628cccf
Cleanup
BenjaminSsempala Apr 9, 2024
973c8ba
Use email instead of ID
BenjaminSsempala Apr 17, 2024
9473b72
Adding required fields
BenjaminSsempala Apr 23, 2024
6f81753
Update src/auth-service/utils/email.msgs.js
BenjaminSsempala Jun 13, 2024
9c6a76d
Update src/auth-service/config/global/email-templates.js
BenjaminSsempala Jun 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 23 additions & 15 deletions src/auth-service/config/global/email-templates.js
View file
Open in desktop
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>
</a>
<span
style="color: #667085; font-size: 14px; font-family: Inter; font-weight: 400; line-height: 20px; word-wrap: break-word;"></span>
<br /><br />
`;
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
72 changes: 34 additions & 38 deletions src/auth-service/models/Subscription.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,32 +19,17 @@
required: true,
unique: true,
},
subscribed: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @BenjaminSsempala , thanks for this. What was the motivation for removing this subscribed and isSystemUser fields? Were they conflicting with anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @Baalmart , I assumed these were place holders since we weren't using them anywhere. I can add them back if required

Copy link
Contributor

Choose a reason for hiding this comment

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

These are not really placeholders, these are fields which will be utilised in the near future. On a side note, I usually do not leave "comments" or "place holder" in most of my code --- I do not consider it as a good practice. @BenjaminSsempala

type: Boolean,
default: true,
},
isSystemUser: {
type: Boolean,
required: true,
},
notifications: {
twitter: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @BenjaminSsempala , what was the motivation for removing Twitter field? Was it conflicting with anything?

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 Same for this one, I assumed this was a place holder, I can add it back if required

Copy link
Contributor

Choose a reason for hiding this comment

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

These are not really placeholders, these are fields which will be utilised in the near future. On a side note, I usually do not leave "comments" or "place holder" in most of my code --- I do not consider it as a good practice. @BenjaminSsempala

type: Boolean,
default: true,
},
email: {
type: Boolean,
default: true,
},
phone: {
type: Boolean,
default: true,
},
sms: {

mobile_push: {
type: Boolean,
default: true,
},
},
}
},
{
timestamps: true,
Expand All @@ -63,8 +48,7 @@
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,
};
},
Expand Down Expand Up @@ -250,8 +234,24 @@
}
};

SubscriptionSchema.statics.subscribe = async function (email, type) {
const updatedSubscription = await this.findOneAndUpdate(

Check warning on line 238 in src/auth-service/models/Subscription.js

View check run for this annotation

Codecov / codecov/patch

src/auth-service/models/Subscription.js#L238 

Added line #L238 was not covered by tests
{ email },
{ $set: { [`notifications.${type}`]: true } },
{ new: true, upsert: true }
);

return updatedSubscription;

Check warning on line 244 in src/auth-service/models/Subscription.js

View check run for this annotation

Codecov / codecov/patch

src/auth-service/models/Subscription.js#L244 

Added line #L244 was not covered by tests
};

SubscriptionSchema.statics.unsubscribe = async function (email, type) {
await this.updateOne({ email }, { [`notifications.${type}`]: false });
const updatedSubscription = await this.findOneAndUpdate(

Check warning on line 248 in src/auth-service/models/Subscription.js

View check run for this annotation

Codecov / codecov/patch

src/auth-service/models/Subscription.js#L248 

Added line #L248 was not covered by tests
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;

Check warning on line 254 in src/auth-service/models/Subscription.js

View check run for this annotation

Codecov / codecov/patch

src/auth-service/models/Subscription.js#L254 

Added line #L254 was not covered by tests
};

SubscriptionSchema.statics.checkNotificationStatus = async function (
Expand All @@ -270,31 +270,27 @@
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];

Check warning on line 275 in src/auth-service/models/Subscription.js

View check run for this annotation

Codecov / codecov/patch

src/auth-service/models/Subscription.js#L275 

Added line #L275 was not covered by tests


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 {

Check warning on line 289 in src/auth-service/models/Subscription.js

View check run for this annotation

Codecov / codecov/patch

src/auth-service/models/Subscription.js#L289 

Added line #L289 was not covered by tests
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
View file
Open in desktop
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