Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds PII (Personally Identifiable Information) alert functionality to the Countly alerts plugin, enabling users to be notified when sensitive data incidents are detected. The feature integrates with an existing PII detection plugin and provides both event-triggered and scheduled alert capabilities.
Changes:
- Added new PII alert module with event-triggered and scheduled check functionality
- Updated frontend UI to support PII data type with rule selection and "All Applications" compatibility
- Added comprehensive end-to-end tests covering CRUD operations, incident creation, and alert triggering
- Updated localization, CSS styling, and validation logic to accommodate PII alerts
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| plugins/alerts/api/alertModules/pii.js | New alert module implementing triggerByEvent and check functions for PII incidents |
| plugins/alerts/api/ingestor.js | Registers /pii/incident hook to trigger PII alerts independently from SDK events |
| plugins/alerts/api/jobs/AlertProcessor.js | Adds PII module to scheduled alert processor |
| plugins/alerts/api/parts/common-lib.js | Adds PII to TRIGGERED_BY_EVENT enum and removes trailing whitespace |
| plugins/alerts/frontend/public/javascripts/countly.views.js | Adds PII data type options, rule fetching, icon mapping, prefill support, and calculateWidth enhancement |
| plugins/alerts/frontend/public/javascripts/countly.models.js | Implements getPiiRulesForApp function to fetch PII rules via API |
| plugins/alerts/frontend/public/templates/vue-main.html | Removes disabled attribute on data type select, fixes whitespace, updates calculateWidth call |
| plugins/alerts/frontend/public/stylesheets/vue-main.scss | Adds shield-exclamation icon styling for PII alerts |
| plugins/alerts/frontend/public/localization/alerts.properties | Adds PII-related localization strings |
| plugins/alerts/tests/pii.js | Comprehensive E2E tests for PII alert CRUD, incident creation, event-triggered and scheduled alerts |
| plugins/alerts/tests/index.js | Includes new PII test file |
| plugins/alerts/tests.js | Adds "pii" to valid data types and adds null checks for filterKey/filterValue |
| Selected data binding into compareType property which is defifen in model.js file--> | ||
|
|
||
| <cly-form-field rules="required" style="display:inline-block" v-if="isCompareTypeSelectAvailable"> | ||
| <cly-form-field rules="required" style="display:inline-block" v-if="isCompareTypeSelectAvailable"> |
There was a problem hiding this comment.
The calculateWidth function now accepts an options parameter to display the label instead of the value (see line 345 for example usage with alertDataSubTypeOptions). However, this call doesn't pass the alertDataVariableOptions parameter. This inconsistency means the width calculation will use the raw value instead of the displayed label, potentially causing incorrect width calculations for this select element.
| const selectedApp = alert.selectedApps[0]; | ||
| let app = null; | ||
| let appId = null; | ||
|
|
||
| if (selectedApp !== "all") { | ||
| app = await common.readBatcher.getOne("apps", { _id: new ObjectId(selectedApp) }); | ||
| if (!app) { | ||
| log.e(`App ${selectedApp} couldn't be found`); | ||
| return; | ||
| } | ||
| appId = app._id.toString(); | ||
| } | ||
|
|
||
| let { period, compareType, compareValue, filterValue, alertDataSubType2 } = alert; | ||
| compareValue = Number(compareValue); | ||
|
|
||
| const metricValue = await countIncidents(date, period, appId, filterValue, alertDataSubType2) || 0; | ||
|
|
||
| if (compareType === commonLib.COMPARE_TYPE_ENUM.MORE_THAN) { | ||
| if (metricValue > compareValue) { | ||
| await commonLib.trigger({ alert, app, metricValue, date }, log); | ||
| } | ||
| } | ||
| else { | ||
| const before = moment(date).subtract(1, commonLib.PERIOD_TO_DATE_COMPONENT_MAP[period]).toDate(); | ||
| const metricValueBefore = await countIncidents(before, period, appId, filterValue, alertDataSubType2); | ||
| if (!metricValueBefore) { | ||
| return; | ||
| } | ||
|
|
||
| const change = (metricValue / metricValueBefore - 1) * 100; | ||
| const shouldTrigger = compareType === commonLib.COMPARE_TYPE_ENUM.INCREASED_BY | ||
| ? change >= compareValue | ||
| : change <= -compareValue; | ||
|
|
||
| if (shouldTrigger) { | ||
| await commonLib.trigger({ alert, app, date, metricValue, metricValueBefore }, log); |
There was a problem hiding this comment.
When selectedApp is "all", the app variable remains null but is still passed to commonLib.trigger at line 96. The trigger function expects an app object to generate email content (it accesses app.name and app._id). This will cause a runtime error when trying to trigger alerts for "all" apps. Consider either fetching a list of all apps when selectedApp is "all", or handling the null app case in the trigger function.
| const selectedApp = alert.selectedApps[0]; | |
| let app = null; | |
| let appId = null; | |
| if (selectedApp !== "all") { | |
| app = await common.readBatcher.getOne("apps", { _id: new ObjectId(selectedApp) }); | |
| if (!app) { | |
| log.e(`App ${selectedApp} couldn't be found`); | |
| return; | |
| } | |
| appId = app._id.toString(); | |
| } | |
| let { period, compareType, compareValue, filterValue, alertDataSubType2 } = alert; | |
| compareValue = Number(compareValue); | |
| const metricValue = await countIncidents(date, period, appId, filterValue, alertDataSubType2) || 0; | |
| if (compareType === commonLib.COMPARE_TYPE_ENUM.MORE_THAN) { | |
| if (metricValue > compareValue) { | |
| await commonLib.trigger({ alert, app, metricValue, date }, log); | |
| } | |
| } | |
| else { | |
| const before = moment(date).subtract(1, commonLib.PERIOD_TO_DATE_COMPONENT_MAP[period]).toDate(); | |
| const metricValueBefore = await countIncidents(before, period, appId, filterValue, alertDataSubType2); | |
| if (!metricValueBefore) { | |
| return; | |
| } | |
| const change = (metricValue / metricValueBefore - 1) * 100; | |
| const shouldTrigger = compareType === commonLib.COMPARE_TYPE_ENUM.INCREASED_BY | |
| ? change >= compareValue | |
| : change <= -compareValue; | |
| if (shouldTrigger) { | |
| await commonLib.trigger({ alert, app, date, metricValue, metricValueBefore }, log); | |
| const selectedApp = alert.selectedApps && alert.selectedApps[0]; | |
| let { period, compareType, compareValue, filterValue, alertDataSubType2 } = alert; | |
| compareValue = Number(compareValue); | |
| // If alert is configured for a specific app, keep existing single-app behavior | |
| if (selectedApp && selectedApp !== "all") { | |
| const app = await common.readBatcher.getOne("apps", { _id: new ObjectId(selectedApp) }); | |
| if (!app) { | |
| log.e(`App ${selectedApp} couldn't be found`); | |
| return; | |
| } | |
| const appId = app._id.toString(); | |
| const metricValue = await countIncidents(date, period, appId, filterValue, alertDataSubType2) || 0; | |
| if (compareType === commonLib.COMPARE_TYPE_ENUM.MORE_THAN) { | |
| if (metricValue > compareValue) { | |
| await commonLib.trigger({ alert, app, metricValue, date }, log); | |
| } | |
| } | |
| else { | |
| const before = moment(date).subtract(1, commonLib.PERIOD_TO_DATE_COMPONENT_MAP[period]).toDate(); | |
| const metricValueBefore = await countIncidents(before, period, appId, filterValue, alertDataSubType2); | |
| if (!metricValueBefore) { | |
| return; | |
| } | |
| const change = (metricValue / metricValueBefore - 1) * 100; | |
| const shouldTrigger = compareType === commonLib.COMPARE_TYPE_ENUM.INCREASED_BY | |
| ? change >= compareValue | |
| : change <= -compareValue; | |
| if (shouldTrigger) { | |
| await commonLib.trigger({ alert, app, date, metricValue, metricValueBefore }, log); | |
| } | |
| } | |
| return; | |
| } | |
| // Handle "all" apps: evaluate and trigger per app, always passing a valid app object | |
| if (selectedApp === "all") { | |
| const appsCursor = common.db.collection("apps").find({}, {projection: {_id: 1, name: 1}}); | |
| const apps = await appsCursor.toArray(); | |
| if (!apps || !apps.length) { | |
| log.w("No apps found while processing PII alerts for 'all' apps"); | |
| return; | |
| } | |
| for (const app of apps) { | |
| const appId = app._id.toString(); | |
| const metricValue = await countIncidents(date, period, appId, filterValue, alertDataSubType2) || 0; | |
| if (compareType === commonLib.COMPARE_TYPE_ENUM.MORE_THAN) { | |
| if (metricValue > compareValue) { | |
| await commonLib.trigger({ alert, app, metricValue, date }, log); | |
| } | |
| } | |
| else { | |
| const before = moment(date).subtract(1, commonLib.PERIOD_TO_DATE_COMPONENT_MAP[period]).toDate(); | |
| const metricValueBefore = await countIncidents(before, period, appId, filterValue, alertDataSubType2); | |
| if (!metricValueBefore) { | |
| continue; | |
| } | |
| const change = (metricValue / metricValueBefore - 1) * 100; | |
| const shouldTrigger = compareType === commonLib.COMPARE_TYPE_ENUM.INCREASED_BY | |
| ? change >= compareValue | |
| : change <= -compareValue; | |
| if (shouldTrigger) { | |
| await commonLib.trigger({ alert, app, date, metricValue, metricValueBefore }, log); | |
| } | |
| } |
Changes for app_users export download
No description provided.