Subsribe and unsubscribe functionality #3035
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## staging #3035 +/- ##
===========================================
+ Coverage 30.31% 31.22% +0.91%
===========================================
Files 188 176 -12
Lines 24647 23650 -997
Branches 3184 3067 -117
===========================================
- Hits 7472 7385 -87
+ Misses 17057 16147 -910
Partials 118 118
|
|
Auth-service changes in this PR available for preview here |
src/auth-service/models/User.js
Outdated
| push: { type: Boolean, default: false }, | ||
| text: { type: Boolean, default: false }, | ||
| phone: { type: Boolean, default: false }, | ||
| mobile_notifications: { |
There was a problem hiding this comment.
this is a good direction @BenjaminSsempala , love it! :)
There was a problem hiding this comment.
Hi @BenjaminSsempala ....on a second thought, we might want to rethink this and just maintain one notifications field.......since it already had "phone" in it, we might not need to create a new field for mobile.
Happy to hear your additional thoughts on the same.
|
Auth-service changes in this PR available for preview here |
| @@ -543,12 +519,6 @@ const mailer = { | |||
| next | |||
| ) => { | |||
| try { | |||
| const checkResult = await SubscriptionModel( | |||
There was a problem hiding this comment.
Hi @BenjaminSsempala , could you shed some light as to why these notification status checks are being removed? Could you share a small high level write up of this approach so that I can easily review this PR? I am a little bit confused to be honest. :)
There was a problem hiding this comment.
Hi @BenjaminSsempala , could you shed more light on why these subscription check are being removed?
There was a problem hiding this comment.
@Baalmart I removed the subscription checks for emails, that we might consider compulsory like forgot password, getting token, verifying email etc
There was a problem hiding this comment.
Okay, thanks @BenjaminSsempala , this makes a lot of sense! Thanks!
There was a problem hiding this comment.
Hi @BenjaminSsempala , thanks for the amazing work, good progress! :)
Just requesting for an online meeting to go over some details that might need addressing as we try to move towards a unified authentication system.
|
Auth-service changes in this PR available for preview here |
| email: this.subscribed, | ||
| isSystemUser: this.isSystemUser, | ||
| notifications: this.notifications, | ||
| email: this.email, |
There was a problem hiding this comment.
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.
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.
|
Auth-service changes in this PR available for preview here |
There was a problem hiding this comment.
Thanks @BenjaminSsempala for this, much appreciated.
I have just made a quick observation that needs your feedback.
src/auth-service/routes/v2/users.js
Outdated
| router.post( | ||
| "/subscribe/:type", | ||
| router.get( | ||
| "/subscribe/:product/:type", |
There was a problem hiding this comment.
Hi @BenjaminSsempala , thanks for this.
- Just curious, why are we still adding
productas a query parameter in our endpoints? - Considering the
one accountdirection, isn'ttypejust enough?
More than happy to dig further into this in a sync of sorts. Thanks again.
There was a problem hiding this comment.
Hello @Baalmart
The type is for differentiating the category of notification i.e email, push, since each of the platforms has these
The product is for differentiating between the platforms mobile, analytics, web, etc but product describes it better.
Even with the one account, the user would still have multiple platforms available to them e.g how slack has both web push notifications and mobile push notifications.
The alternative would be hardcoding the product and type as one i.e mobile-email, then mobile-push which might be repetitive, especially for future maintenance.
| email: { | ||
| type: Boolean, | ||
| default: true, | ||
| mobile: { |
There was a problem hiding this comment.
Hi @BenjaminSsempala , just curious, could there be any challenges with completely letting go of the explicit product categorisations? If so, I would like to appreciate. My worry at the moment is that we might struggle to achieve the true one account experience if we continue with the mental model of product based categorisations.
There was a problem hiding this comment.
Hello @Baalmart I was looking at it from an angle where the product defines the product the account is being used on, which would allow us or users to have more granular control of their notification preferences.
I think as we add more notifications, we can adjust the structure accordingly to something that better categorizes the notifications we have.
Example
|
Auth-service changes in this PR available for preview here |
There was a problem hiding this comment.
Thanks @BenjaminSsempala ,
- The alignment is now definitely reaching its final phase! Brilliant stuff. :)
- I have gone ahead to leave a few comments for your review.
src/auth-service/utils/email.msgs.js
Outdated
| @@ -232,9 +233,9 @@ module.exports = { | |||
| style="color: #135DFF; font-size: 14px; font-family: Inter; font-weight: 400; line-height: 20px; word-wrap: break-word;">[email protected]</span> | |||
| </td> | |||
| </tr>`; | |||
| return constants.EMAIL_BODY(email, content, name); | |||
| return constants.EMAIL_BODY(email, content, name, "email", `email=${email}&mongo_user_id=${user_id}`); | |||
There was a problem hiding this comment.
Thanks for this @BenjaminSsempala , could you share some background notes to this mongo_user_id?
There was a problem hiding this comment.
Hey @Baalmart This was being used initially as an alternative for the email, but I've now removed all the usages of the ids
| @@ -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.
Thanks for this @BenjaminSsempala ,
- Could you share some background notes to this variable declaration? I though
environmentspecific variables are handled underconfig/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_URLorconstants.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.
There was a problem hiding this comment.
@Baalmart , Yes, I failed to use the constants variable because of circular dependencies.
There was a problem hiding this comment.
Thanks @BenjaminSsempala ,I have left a few comment for your review. Thanks a lot for the work, this is AMAZING!
| @@ -19,32 +19,17 @@ const SubscriptionSchema = new mongoose.Schema( | |||
| required: true, | |||
| unique: true, | |||
| }, | |||
| subscribed: { | |||
There was a problem hiding this comment.
Hi @BenjaminSsempala , thanks for this. What was the motivation for removing this subscribed and isSystemUser fields? Were they conflicting with anything?
There was a problem hiding this comment.
Hello @Baalmart , I assumed these were place holders since we weren't using them anywhere. I can add them back if required
There was a problem hiding this comment.
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
| notifications: { | ||
| twitter: { |
There was a problem hiding this comment.
Hi @BenjaminSsempala , what was the motivation for removing Twitter field? Was it conflicting with anything?
There was a problem hiding this comment.
@Baalmart Same for this one, I assumed this was a place holder, I can add it back if required
There was a problem hiding this comment.
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
| 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.
Great work here, thanks @BenjaminSsempala
| @@ -543,12 +519,6 @@ const mailer = { | |||
| next | |||
| ) => { | |||
| try { | |||
| const checkResult = await SubscriptionModel( | |||
There was a problem hiding this comment.
Hi @BenjaminSsempala , could you shed more light on why these subscription check are being removed?
| firebase_result = result.writeTime ? true : false; | ||
|
|
||
|
|
||
| if (!isEmpty(userDoc.data().analyticsMongoID)) { |
There was a problem hiding this comment.
Hi @BenjaminSsempala , thanks for this. Could you briefly explain the changes and their objectives? Specifically, I'm interested in the rationale behind the conditional checks and whether this approach supports our "one Account" model. It seems there's a trend towards categorising users by product, which might conflict with our goal. To align with our "one Account" philosophy, I suggest we focus on using email exclusively for all verification processes, including Firebase checks. This would streamline user management by relying solely on email for authentication and verification checks in any data storage system (Firebase, Mongo, Postgresql, MySQL, etc).
There was a problem hiding this comment.
@Baalmart Thanks, I've modified this and other usages to check for email instead of checking for the ids
There was a problem hiding this comment.
This is now looking brilliant! Thanks @BenjaminSsempala I have just left a few comments for your review.
| @@ -19,32 +19,17 @@ const SubscriptionSchema = new mongoose.Schema( | |||
| required: true, | |||
| unique: true, | |||
| }, | |||
| subscribed: { | |||
There was a problem hiding this comment.
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
| notifications: { | ||
| twitter: { |
There was a problem hiding this comment.
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
| @@ -543,12 +519,6 @@ const mailer = { | |||
| next | |||
| ) => { | |||
| try { | |||
| const checkResult = await SubscriptionModel( | |||
There was a problem hiding this comment.
Okay, thanks @BenjaminSsempala , this makes a lot of sense! Thanks!
|
#3035 (comment) |
|
@coderabbitai review |
Actions performedReview triggered.
|
WalkthroughWalkthroughThe recent changes enhance the Changes
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range and nitpick comments (9)
src/auth-service/models/Subscription.js (1)
41-44: Add index to improve query performance.Given that
mobile_pushis likely to be queried frequently, consider adding an index to this field to improve the performance of database operations.src/auth-service/utils/email.templates.js (1)
Line range hint
111-129: Ensure consistency in string processing.The function
processStringis called multiple times on the same variableentity_title. Consider processing it once at the start of the function and reusing the result to improve performance and reduce redundancy.src/auth-service/routes/v2/users.js (3)
Line range hint
1052-1091: Consider validating thetypeparameter against expected types.The
typeparameter in the subscription route is currently validated only for non-emptiness and trimmed. It would be beneficial to also validate it against a list of expected types (e.g., 'email', 'push') to ensure that only valid subscription types can be processed. This would enhance the robustness of the endpoint against invalid inputs.
Line range hint
1096-1135: Consider validating thetypeparameter against expected types.Similar to the subscription route, the
typeparameter in the unsubscribe route is validated for non-emptiness and trimmed but not against a list of expected types. Adding such validation can prevent processing requests with invalid types, thereby enhancing endpoint security and functionality.
Line range hint
24-25: UseNumber.isNaNinstead ofisNaNfor more accurate checks.- req.query.limit = isNaN(limit) || limit < 1 ? 1000 : limit; - req.query.skip = isNaN(skip) || skip < 0 ? 0 : skip; + req.query.limit = Number.isNaN(limit) || limit < 1 ? 1000 : limit; + req.query.skip = Number.isNaN(skip) || skip < 0 ? 0 : skip;Using
Number.isNaNprovides a more accurate check as it does not attempt type coercion, which can lead to unexpected behaviors.src/auth-service/utils/mailer.js (4)
Line range hint
1545-1588: Consider refining the error handling insendUnsubscriptionEmail.The error handling pattern used here is consistent with other parts of the module. However, consider adding more specific error messages or categorization to help with debugging and maintenance. For example, distinguishing between different types of email sending failures could help in troubleshooting.
Line range hint
1987-1987: Address the invalid assignment in the function signature.- { email = "", firstName = "", lastName = "", tenant = "airqo" = "" } = {}, + { email = "", firstName = "", lastName = "", tenant = "airqo" } = {},The default assignment to
"airqo"in the function parameters is incorrect. Ensure that the default values are set properly without reassignment syntax.
Line range hint
54-54: Reorder default parameters in function signatures.Several functions in this file have default parameters that precede required parameters, which can lead to issues when calling these functions without specifying all arguments. Consider reordering the parameters so that default parameters follow required parameters.
Also applies to: 129-130, 208-215, 306-308, 414-416, 497-499, 599-607, 722-725, 840-845, 914-922
Line range hint
105-116: Remove unnecessary else clauses.There are several
elseclauses in this file that can be omitted because the preceding branches of code (e.g.,returnstatements) ensure that the code following theelsewill not be executed unless the condition is met. This can help reduce the complexity and improve the readability of the code.Also applies to: 183-195, 273-293, 391-402, 474-485, 700-711, 818-829, 890-901
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
src/auth-service/package-lock.jsonis excluded by!**/package-lock.json
Files selected for processing (8)
- src/auth-service/config/global/email-templates.js (4 hunks)
- src/auth-service/models/Subscription.js (5 hunks)
- src/auth-service/models/User.js (1 hunks)
- src/auth-service/routes/v2/users.js (7 hunks)
- src/auth-service/utils/create-user.js (7 hunks)
- src/auth-service/utils/email.msgs.js (19 hunks)
- src/auth-service/utils/email.templates.js (7 hunks)
- src/auth-service/utils/mailer.js (16 hunks)
Files not reviewed due to errors (1)
- src/auth-service/utils/create-user.js (no review received)
Additional context used
Biome
src/auth-service/config/global/email-templates.js
[error] 7-14: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 15-27: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 28-85: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.src/auth-service/models/Subscription.js
[error] 64-66: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 85-85: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
[error] 100-107: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 120-120: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 127-127: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 137-138: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.
[error] 154-162: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 173-174: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.
[error] 181-182: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
[error] 197-205: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 220-221: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.
[error] 241-249: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
src/auth-service/models/User.js
[error] 121-124: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 150-153: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 207-209: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 211-213: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 309-316: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 324-324: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 328-328: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 415-423: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 434-434: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.
[error] 442-443: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
[error] 621-629: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 640-641: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.
[error] 655-656: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
[error] 660-661: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
[error] 666-667: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
[error] 671-672: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
[error] 679-682: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
[error] 695-702: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 713-714: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.
[error] 733-740: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
src/auth-service/routes/v2/users.js
[error] 24-24: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead. (lint/suspicious/noGlobalIsNan)
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
[error] 25-25: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead. (lint/suspicious/noGlobalIsNan)
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.src/auth-service/utils/email.msgs.js
[error] 348-348: Invalid assignment to
""(parse)This expression cannot be assigned to
[error] 348-348: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.src/auth-service/utils/mailer.js
[error] 1987-1987: Invalid assignment to
"airqo"(parse)This expression cannot be assigned to
[error] 54-54: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.
[error] 105-116: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 129-130: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.
[error] 183-195: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 208-215: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.
[error] 273-293: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 306-308: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.
[error] 391-402: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 414-416: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.
[error] 474-485: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 497-499: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.
[error] 574-585: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 599-607: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.
[error] 700-711: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 722-725: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.
[error] 818-829: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 840-845: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.
[error] 890-901: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 914-922: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.src/auth-service/utils/create-user.js
[error] 93-93: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.
[error] 181-181: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.
[error] 229-239: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 271-286: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 526-544: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 441-446: This code is unreachable (lint/correctness/noUnreachable)
... because this statement will return from the function beforehand
[error] 446-465: This code is unreachable (lint/correctness/noUnreachable)
... because this statement will return from the function beforehand
[error] 465-478: This code is unreachable (lint/correctness/noUnreachable)
... because this statement will return from the function beforehand
[error] 478-479: This code is unreachable (lint/correctness/noUnreachable)
... because this statement will return from the function beforehand
[error] 479-486: This code is unreachable (lint/correctness/noUnreachable)
... because this statement will return from the function beforehand
[error] 486-506: This code is unreachable (lint/correctness/noUnreachable)
... because this statement will return from the function beforehand
[error] 507-526: This code is unreachable (lint/correctness/noUnreachable)
... because this statement will return from the function beforehand
[error] 526-544: This code is unreachable (lint/correctness/noUnreachable)
... because this statement will return from the function beforehand
[error] 594-595: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
[error] 598-598: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
[error] 629-647: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 643-647: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 892-930: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 977-1009: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 1035-1035: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.
GitHub Check: codecov/patch
src/auth-service/config/global/email-templates.js
[warning] 28-29: src/auth-service/config/global/email-templates.js#L28-L29
Added lines #L28 - L29 were not covered by tests
[warning] 31-32: src/auth-service/config/global/email-templates.js#L31-L32
Added lines #L31 - L32 were not covered by tests
[warning] 87-88: src/auth-service/config/global/email-templates.js#L87-L88
Added lines #L87 - L88 were not covered by tests
Additional comments not posted (11)
src/auth-service/models/Subscription.js (1)
260-277: Refactor to reduce duplication in subscription methods.
[REFACTOR_SUGGESTion]- 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) { - const updatedSubscription = await this.findOneAndUpdate( - { email }, - { $set: { [`notifications.${type}`]: false } }, - { new: true, upsert: true } - ); - return updatedSubscription; - }; + SubscriptionSchema.statics.updateSubscription = async function (email, type, isSubscribed) { + const updatedSubscription = await this.findOneAndUpdate( + { email }, + { $set: { [`notifications.${type}`]: isSubscribed } }, + { new: true, upsert: true } + ); + return updatedSubscription; + };This refactoring reduces code duplication by combining the
subscribeandunsubscribemethods into a single method that takes a booleanisSubscribedparameter.src/auth-service/models/User.js (9)
Line range hint
1-1: Ensure all necessary modules are correctly imported.
Line range hint
121-124: Refactor this function expression to use an arrow function for better readability and consistency.
[REFACTOR_SUGGESTion]- function oneMonthFromNow() { + const oneMonthFromNow = () => {
Line range hint
150-153: Convert this validator function into an arrow function to maintain consistency and improve readability.
[REFACTOR_SUGGESTion]- validator: function (value) { + validator: (value) => {
Line range hint
207-209: These validator functions should be converted to arrow functions for consistency and to avoid unnecessary function scope binding.
[REFACTOR_SUGGESTion]- validator: function (value) { + validator: (value) => {Also applies to: 211-213
Line range hint
309-316: Theelseclause is unnecessary due to early returns in the previous branches. Removing it can simplify the code.
[REFACTOR_SUGGESTion]- else { - throw new HttpError( - "Internal Server Error", - httpStatus.INTERNAL_SERVER_ERROR, - { - message: - "Contact [email protected] -- unable to retrieve the default Network or Role to which the User will belong", - } - ); - }
Line range hint
415-423: Similar to previous comments, theelseclause here can be omitted for cleaner and more readable code.
[REFACTOR_SUGGESTion]- else { - throw new HttpError( - "Internal Server Error", - httpStatus.INTERNAL_SERVER_ERROR, - { - message: - "Contact [email protected] -- unable to retrieve the default Group or Role to which the User will belong", - } - ); - }
Line range hint
442-443: Avoid using thedeleteoperator in expressions as it can impact performance. Consider setting the property toundefinedinstead.
[REFACTOR_SUGGESTion]- delete modifiedUpdate.network_roles; + modifiedUpdate.network_roles = undefined;Also applies to: 655-656, 660-661, 666-667, 671-672, 679-682
Line range hint
695-702: Another instance where theelseclause can be omitted due to early returns.
[REFACTOR_SUGGESTion]- else { - next( - new HttpError("Bad Request Error", httpStatus.BAD_REQUEST, { - message: "Provided User does not exist, please crosscheck", - }) - ); - }
Line range hint
713-714: Ensure the default parameter follows the last required parameter or make it a required parameter.
[REFACTOR_SUGGESTion]- const UserModel = (tenant = "airqo") => { + const UserModel = (tenant) => {src/auth-service/utils/mailer.js (1)
1528-1542: Review the newsendUnsubscriptionEmailfunction for correctness and efficiency.The implementation of this function appears to follow the existing patterns in the module and uses the new
emailNotificationUnsubscibemessage function correctly. Ensure that theparamStringandtypeparameters are correctly validated upstream before being used here to prevent any potential injection or formatting issues.
| @@ -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.
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.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/auth-service/utils/email.msgs.js (19 hunks)
Additional comments not posted (2)
src/auth-service/utils/email.msgs.js (2)
36-36: Refactor to enhance maintainability and readability.As previously suggested, consider abstracting the repeated pattern of constructing email bodies into a helper function. This change would improve code maintainability and readability.
Also applies to: 54-54, 75-75, 162-162, 175-175, 188-188, 203-203, 236-236, 260-260, 294-294, 326-326, 346-346, 359-359, 379-379, 398-398, 417-417, 434-434, 451-451, 473-473, 502-502
478-502: Review the unsubscribe functionality for potential improvements.
[REFACTOR_SUGGESTion]
The content of the unsubscribe email could be improved for clarity. The message "You will no longer receive updates and notifications from your favorite locations" is repeated twice. Consider revising the email template to avoid redundancy and enhance the user experience.- You will no longer receive updates and notifications from your favorite locations. If you ever change your mind and want to rejoin us, feel free to subscribe again at any time. + If you ever change your mind and want to rejoin us, feel free to subscribe again at any time.Also, ensure that the subscription URL is correctly formed and secure, especially in how
paramStringis handled to prevent injection attacks.
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
src/auth-service/config/global/email-templates.js (1)
Line range hint
28-85: Refactor to use an arrow function for consistency and clarity.- EMAIL_FOOTER_TEMPLATE: function (email, type, paramString) { + EMAIL_FOOTER_TEMPLATE: (email, type, paramString) => {This change aligns with modern JavaScript practices and enhances the readability of your code. It also addresses the lint error regarding the use of arrow functions.
Tools
GitHub Check: codecov/patch
[warning] 28-29: src/auth-service/config/global/email-templates.js#L28-L29
Added lines #L28 - L29 were not covered by tests
[warning] 31-32: src/auth-service/config/global/email-templates.js#L31-L32
Added lines #L31 - L32 were not covered by tests
[warning] 37-37: src/auth-service/config/global/email-templates.js#L37
Added line #L37 was not covered by tests
[warning] 41-42: src/auth-service/config/global/email-templates.js#L41-L42
Added lines #L41 - L42 were not covered by tests
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/auth-service/config/global/email-templates.js (4 hunks)
Additional context used
GitHub Check: codecov/patch
src/auth-service/config/global/email-templates.js
[warning] 28-29: src/auth-service/config/global/email-templates.js#L28-L29
Added lines #L28 - L29 were not covered by tests
[warning] 31-32: src/auth-service/config/global/email-templates.js#L31-L32
Added lines #L31 - L32 were not covered by tests
[warning] 37-37: src/auth-service/config/global/email-templates.js#L37
Added line #L37 was not covered by tests
[warning] 41-42: src/auth-service/config/global/email-templates.js#L41-L42
Added lines #L41 - L42 were not covered by tests
[warning] 86-87: src/auth-service/config/global/email-templates.js#L86-L87
Added lines #L86 - L87 were not covered by tests
| const baseUrl = process.env.PLATFORM_STAGING_BASE_URL || process.env.PLATFORM_PRODUCTION_BASE_URL; | ||
|
|
There was a problem hiding this comment.
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.
| 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 /> | ||
| `; | ||
| } |
There was a problem hiding this comment.
LGTM! Please add test coverage for the dynamic URL generation.
The changes to the EMAIL_FOOTER_TEMPLATE function look good and correctly handle the dynamic generation of the unsubscribe link based on the provided parameters.
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 EMAIL_FOOTER_TEMPLATE? I can provide guidance or even generate the test code for you.
Tools
GitHub Check: codecov/patch
[warning] 28-29: src/auth-service/config/global/email-templates.js#L28-L29
Added lines #L28 - L29 were not covered by tests
[warning] 31-32: src/auth-service/config/global/email-templates.js#L31-L32
Added lines #L31 - L32 were not covered by tests
[warning] 37-37: src/auth-service/config/global/email-templates.js#L37
Added line #L37 was not covered by tests
[warning] 41-42: src/auth-service/config/global/email-templates.js#L41-L42
Added lines #L41 - L42 were not covered by tests
| 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 === "") { |
There was a problem hiding this comment.
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
WHAT DOES THIS PR DO?
HOW DO I TEST OUT THIS PR?
WHICH ENDPOINTS SHOULD BE READY FOR TESTING?:
ARE THERE ANY RELATED PRs?
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
POSTtoGETfor subscription-related routes.Chores