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

automated update of default preferences (selected-sites) #3644

Merged
merged 3 commits into from
Oct 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
70 changes: 50 additions & 20 deletions src/auth-service/bin/jobs/preferences-update-job.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
const cron = require("node-cron");
const UserModel = require("@models/User");
const PreferenceModel = require("@models/Preference");
const SelectedSiteModel = require("@models/SelectedSite");
const constants = require("@config/constants");
const log4js = require("log4js");
const { logText, logObject } = require("@utils/log");
Expand All @@ -9,9 +10,7 @@ const logger = log4js.getLogger(
);
const stringify = require("@utils/stringify");
const isEmpty = require("is-empty");

// Predefined array of 4 site IDs
const defaultSiteIds = constants.SELECTED_SITES;
const BATCH_SIZE = 100;

// Default preference object
const defaultPreference = {
Expand All @@ -28,17 +27,53 @@ const defaultPreference = {
unitValue: 14,
unit: "day",
},
airqloud_id: constants.DEFAULT_AIRQLOUD,
grid_id: constants.DEFAULT_GRID,
network_id: constants.DEFAULT_NETWORK,
group_id: constants.DEFAULT_GROUP,
airqloud_id: constants.DEFAULT_AIRQLOUD || "NA",
grid_id: constants.DEFAULT_GRID || "NA",
network_id: constants.DEFAULT_NETWORK || "NA",
group_id: constants.DEFAULT_GROUP || "NA",
};

const updatePreferences = async () => {
// Function to get selected sites based on the specified method
const getSelectedSites = async (method = "featured") => {
try {
const batchSize = 100;
let selectedSites;
if (method === "featured") {
selectedSites = await SelectedSiteModel("airqo")
.find({ isFeatured: true })
.sort({ createdAt: -1 })
.limit(4)
.lean();
} else {
selectedSites = await SelectedSiteModel("airqo")
.find()
.sort({ createdAt: -1 })
.limit(4)
.lean();
}
Comment on lines +40 to +52
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refactor getSelectedSites to Eliminate Code Duplication

The if...else blocks in the getSelectedSites function contain duplicated query logic. Refactoring this section can enhance readability and maintainability by reducing repetition.

Consider the following refactored version:

const getSelectedSites = async (method = "featured") => {
  try {
    const query = method === "featured" ? { isFeatured: true } : {};
    const selectedSites = await SelectedSiteModel("airqo")
      .find(query)
      .sort({ createdAt: -1 })
      .limit(4)
      .lean();

    const modifiedSelectedSites = selectedSites.map((site) => ({
      ...site,
      siteId: site.site_id || null,
    }));
    return modifiedSelectedSites;
  } catch (error) {
    logger.error(`🐛🐛 Error fetching selected sites: ${stringify(error)}`);
    return [];
  }
};

const modifiedSelectedSites = selectedSites.map((site) => ({
...site,
_id: site.site_id || null,
}));
Comment on lines +53 to +56
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid Overwriting the MongoDB _id Field

Overwriting the _id field with site.site_id may lead to unexpected behavior since _id is a special field used by MongoDB to uniquely identify documents. It's advisable to use a different field name to store site_id to prevent any potential conflicts.

Here's a suggested change:

-    const modifiedSelectedSites = selectedSites.map((site) => ({
-      ...site,
-      _id: site.site_id || null,
-    }));
+    const modifiedSelectedSites = selectedSites.map((site) => ({
+      ...site,
+      siteId: site.site_id || null,
+    }));
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const modifiedSelectedSites = selectedSites.map((site) => ({
...site,
_id: site.site_id || null,
}));
const modifiedSelectedSites = selectedSites.map((site) => ({
...site,
siteId: site.site_id || null,
}));

return modifiedSelectedSites;
} catch (error) {
logger.error(`🐛🐛 Error fetching selected sites: ${stringify(error)}`);
return [];
}
};

const updatePreferences = async (siteSelectionMethod = "featured") => {
try {
const batchSize = BATCH_SIZE;
let skip = 0;

// Fetch selected sites data
const selectedSites = await getSelectedSites(siteSelectionMethod);

if (isEmpty(selectedSites) || selectedSites.length < 4) {
logger.error("🐛🐛 No selected sites found. Aborting preference update.");
return;
}
Comment on lines +72 to +75
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Clarify Error Message and Consider Handling Fewer Selected Sites

The current logic aborts the update if fewer than four selected sites are found, which might not be necessary. Additionally, the error message could be misleading when selectedSites is not empty but has less than four entries.

You might want to adjust the code to proceed with the available sites and update the log message:

-    if (isEmpty(selectedSites) || selectedSites.length < 4) {
-      logger.error("🐛🐛 No selected sites found. Aborting preference update.");
-      return;
-    }
+    if (isEmpty(selectedSites)) {
+      logger.error("🐛🐛 No selected sites found. Aborting preference update.");
+      return;
+    } else if (selectedSites.length < 4) {
+      logger.warn(`🐛🐛 Only ${selectedSites.length} selected sites found. Proceeding with available sites.`);
+    }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (isEmpty(selectedSites) || selectedSites.length < 4) {
logger.error("🐛🐛 No selected sites found. Aborting preference update.");
return;
}
if (isEmpty(selectedSites)) {
logger.error("🐛🐛 No selected sites found. Aborting preference update.");
return;
} else if (selectedSites.length < 4) {
logger.warn(`🐛🐛 Only ${selectedSites.length} selected sites found. Proceeding with available sites.`);
}


while (true) {
const users = await UserModel("airqo")
.find()
Expand All @@ -59,16 +94,11 @@ const updatePreferences = async () => {
.lean();

const preferencesMap = new Map();

preferences.forEach((pref) => {
preferencesMap.set(pref.user_id.toString(), pref);
});

// Initialize selected_sites data
const selectedSitesData = defaultSiteIds.map((siteId) => ({
_id: siteId,
createdAt: new Date(),
}));

for (const user of users) {
const userIdStr = user._id.toString();
const preference = preferencesMap.get(userIdStr);
Expand All @@ -79,11 +109,11 @@ const updatePreferences = async () => {
.create({
...defaultPreference,
user_id: user._id,
selected_sites: selectedSitesData,
selected_sites: selectedSites,
})
.catch((error) => {
logger.error(
`Failed to create preference for user ${userIdStr}: ${stringify(
`🐛🐛 Failed to create preference for user ${userIdStr}: ${stringify(
error
)}`
);
Expand All @@ -96,14 +126,14 @@ const updatePreferences = async () => {
{
$set: {
...defaultPreference,
selected_sites: selectedSitesData,
selected_sites: selectedSites,
},
},
{ new: true }
)
.catch((error) => {
logger.error(
`Failed to update preference for user ${userIdStr}: ${stringify(
`🐛🐛 Failed to update preference for user ${userIdStr}: ${stringify(
error
)}`
);
Expand All @@ -120,7 +150,7 @@ const updatePreferences = async () => {
};

const schedule = "30 * * * *"; // At minute 30 of every hour
cron.schedule(schedule, updatePreferences, {
cron.schedule(schedule, () => updatePreferences("featured"), {
scheduled: true,
timezone: "Africa/Nairobi",
});
162 changes: 162 additions & 0 deletions src/auth-service/bin/jobs/test/ut_active-status-job.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,162 @@
require("module-alias/register");
const sinon = require("sinon");
const chai = require("chai");
const expect = chai.expect;
const sinonChai = require("sinon-chai");

describe("checkStatus", () => {
let UserModel;

beforeEach(() => {
// Set up mocks
UserModel = sinon.mock(UserModel);

sinon.stub(UserModel.prototype, "find").resolves(
[
{
_id: "user1",
lastLogin: new Date("2023-01-01T00:00:00Z"),
isActive: true,
},
{
_id: "user2",
lastLogin: new Date("2023-02-01T00:00:00Z"),
isActive: true,
},
{ _id: "user3", lastLogin: null, isActive: true },
{
_id: "user4",
lastLogin: new Date("2023-03-01T00:00:00Z"),
isActive: false,
},
{
_id: "user5",
lastLogin: new Date("2023-04-01T00:00:00Z"),
isActive: true,
},
].slice(0, 100)
);

sinon
.stub(UserModel.prototype, "updateMany")
.resolves({ modifiedCount: 5 });

sinon.stub(console, "error");
sinon.stub(stringify, "default").returns(JSON.stringify({}));
});

afterEach(() => {
// Restore mocks
UserModel.restore();
console.error.restore();
stringify.default.restore();
});

describe("successful execution", () => {
it("should mark inactive users and log results", async () => {
await checkStatus();

expect(UserModel.prototype.find).to.have.been.calledThrice;
expect(UserModel.prototype.updateMany).to.have.been.calledWith(
{ _id: { $in: ["user1", "user2", "user3"] } },
{ isActive: false }
);
expect(console.error).to.not.have.been.called;
});
});

describe("no inactive users found", () => {
it("should not update any users when no inactive users are found", async () => {
sinon.stub(UserModel.prototype, "find").resolves(
[
{
_id: "user1",
lastLogin: new Date("2023-05-01T00:00:00Z"),
isActive: true,
},
{
_id: "user2",
lastLogin: new Date("2023-06-01T00:00:00Z"),
isActive: true,
},
].slice(0, 100)
);

await checkStatus();

expect(UserModel.prototype.updateMany).to.not.have.been.called;
});
});

describe("inactive threshold exceeded", () => {
it("should mark users inactive based on last login time", async () => {
sinon.stub(Date.now, "bind").returns(1697865600000); // Current timestamp
sinon.stub(UserModel.prototype, "find").resolves(
[
{
_id: "user1",
lastLogin: new Date("2023-01-01T00:00:00Z"),
isActive: true,
},
{
_id: "user2",
lastLogin: new Date("2023-02-01T00:00:00Z"),
isActive: true,
},
{ _id: "user3", lastLogin: null, isActive: true },
{
_id: "user4",
lastLogin: new Date("2023-03-01T00:00:00Z"),
isActive: true,
},
].slice(0, 100)
);

await checkStatus();

expect(UserModel.prototype.updateMany).to.have.been.calledWith(
{ _id: { $in: ["user1", "user2", "user3"] } },
{ isActive: false }
);
});
});

describe("internal server error", () => {
it("should log internal server error when executing the function fails", async () => {
sinon.stub(UserModel.prototype, "find").throws(new Error("Test error"));

await checkStatus();

expect(console.error).to.have.been.calledWith(
`Internal Server Error --- Test error`
);
});
});

describe("isActive false users", () => {
it("should skip already inactive users", async () => {
sinon.stub(UserModel.prototype, "find").resolves(
[
{
_id: "user1",
lastLogin: new Date("2023-01-01T00:00:00Z"),
isActive: false,
},
{
_id: "user2",
lastLogin: new Date("2023-02-01T00:00:00Z"),
isActive: true,
},
{ _id: "user3", lastLogin: null, isActive: true },
].slice(0, 100)
);

await checkStatus();

expect(UserModel.prototype.updateMany).to.have.been.calledWith(
{ _id: { $in: ["user2", "user3"] } },
{ isActive: false }
);
});
});
});
Loading
Loading