fix(import): TAMOC-386: Apply same unique restrictions on user admin panel to user reference data import#9256
Closed
dannash100 wants to merge 9 commits intomainfrom
Closed
fix(import): TAMOC-386: Apply same unique restrictions on user admin panel to user reference data import#9256dannash100 wants to merge 9 commits intomainfrom
dannash100 wants to merge 9 commits intomainfrom
Conversation
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances data integrity by enforcing consistent uniqueness constraints for user emails and display names across both the admin panel and reference data import functionalities. It centralizes validation logic into a reusable utility, streamlining user management and preventing duplicate entries from various sources. Additionally, it includes a cleanup of unused invoice product loading features, contributing to a more focused and maintainable codebase. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a centralized function checkUserUniqueness to enforce unique emails and display names for users, and applies it to the user import process and user admin panel APIs. This is a great refactoring that reduces code duplication. I've left a comment to improve the new uniqueness check by making email validation case-insensitive.
However, this PR also includes significant, undocumented changes, such as the removal of the invoiceProductLoader and a drastic simplification of the drugLoader, which removes functionality related to facility-specific drug stock. These changes are outside the scope of the PR's title and should be explained in the description. I've added comments requesting clarification on these removals.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/central-server/app/admin/referenceDataImporter/loaders.js (796-866)
The invoiceProductLoader function has been completely removed. Similar to the changes in drugLoader, this is a significant modification to the importer's functionality that is not mentioned in the pull request description. Could you please add some context about this removal?
packages/central-server/app/admin/userValidation.js (6-8)
The email uniqueness check is case-sensitive, which is likely not the desired behavior. Emails are generally treated as case-insensitive. For consistency with the displayName check and to prevent duplicate users with different email casing (e.g., 'test@example.com' and 'Test@example.com'), you should use a case-insensitive comparison here as well.
const where = { email: { [Op.iLike]: email } };
if (excludeId) where.id = { [Op.ne]: excludeId };
dupes.email = !!(await User.findOne({ where }));
![review-hero[bot]](https://avatars.githubusercontent.com/in/2896273?s=60&v=4){kind=small}
|
🦸 Review Hero Summary |
Co-Authored-By: Review Hero <contact@bes.au>
|
🦸 Review Hero Auto-Fix Skipped 4 comments (replied on each thread). |
Contributor
Author
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Dropped
errModelargument silently disables import error stats- Restored the second errModel argument to all pushError calls in permissionLoader, taskSetLoader, userLoader, taskTemplateLoader, medicationTemplateLoader, procedureTypeLoader, and invoiceProductLoader to re-enable per-model error statistics tracking.
Or push these changes by commenting:
@cursor push 217b1aee68
Preview (217b1aee68)
diff --git a/packages/central-server/app/admin/referenceDataImporter/loaders.js b/packages/central-server/app/admin/referenceDataImporter/loaders.js
--- a/packages/central-server/app/admin/referenceDataImporter/loaders.js
+++ b/packages/central-server/app/admin/referenceDataImporter/loaders.js
@@ -270,7 +270,7 @@
await validateObjectId(
{ ...item, noun: normalizedNoun, objectId: normalizedObjectId },
models,
- pushError,
+ message => pushError(message, 'Permission'),
);
// Any non-empty value in the role cell would mean the role
@@ -342,7 +342,7 @@
}).then(tasks => tasks.map(({ id }) => id));
const nonExistentTaskIds = taskIds.filter(taskId => !existingTaskIds.includes(taskId));
if (nonExistentTaskIds.length > 0) {
- pushError(`Tasks ${nonExistentTaskIds.join(', ')} not found`);
+ pushError(`Tasks ${nonExistentTaskIds.join(', ')} not found`, 'TaskSet');
}
if (!existingTaskIds.length) return [];
@@ -378,8 +378,8 @@
displayName: otherFields.displayName,
excludeId: id,
});
- if (duplicates.email) pushError(`Email "${otherFields.email}" is already in use by another user`);
- if (duplicates.displayName) pushError(`Display name "${otherFields.displayName}" is already in use by another user`);
+ if (duplicates.email) pushError(`Email "${otherFields.email}" is already in use by another user`, 'User');
+ if (duplicates.displayName) pushError(`Display name "${otherFields.displayName}" is already in use by another user`, 'User');
const allowedFacilityIds = allowedFacilities
? allowedFacilities.split(',').map(t => t.trim())
@@ -440,11 +440,11 @@
for (const designation of designationIds) {
const existingData = await models.ReferenceData.findByPk(designation);
if (!existingData) {
- pushError(`Designation "${designation}" does not exist`);
+ pushError(`Designation "${designation}" does not exist`, 'User');
continue;
}
if (existingData.visibilityStatus !== VISIBILITY_STATUSES.CURRENT) {
- pushError(`Designation "${designation}" doesn't have visibilityStatus of current`);
+ pushError(`Designation "${designation}" doesn't have visibilityStatus of current`, 'User');
continue;
}
rows.push({
@@ -498,7 +498,7 @@
);
for (const designationId of designationIds) {
if (!existingDesignationIds.includes(designationId)) {
- pushError(`Designation "${designationId}" does not exist`);
+ pushError(`Designation "${designationId}" does not exist`, 'TaskTemplate');
continue;
}
rows.push({
@@ -625,11 +625,17 @@
where: { id: drugReferenceDataId, type: REFERENCE_TYPES.DRUG },
});
if (!drug) {
- pushError(`Drug with ID "${drugReferenceDataId}" does not exist.`);
+ pushError(
+ `Drug with ID "${drugReferenceDataId}" does not exist.`,
+ 'ReferenceMedicationTemplate',
+ );
}
if (isNaN(doseAmount) && doseAmount?.toString().toLowerCase() !== 'variable') {
- pushError(`Dose amount must be a number or the string "variable".`);
+ pushError(
+ `Dose amount must be a number or the string "variable".`,
+ 'ReferenceMedicationTemplate',
+ );
}
const existingTemplate = await models.ReferenceMedicationTemplate.findOne({
@@ -746,6 +752,7 @@
if (nonExistentSurveyIds.length > 0) {
pushError(
`Linked survey${nonExistentSurveyIds.length > 1 ? 's' : ''} "${nonExistentSurveyIds.join(', ')}" for procedure type "${id}" not found.`,
+ 'ProcedureTypeSurvey',
);
}
@@ -754,6 +761,7 @@
if (nonProgramSurveys.length > 0) {
pushError(
`Survey${nonProgramSurveys.length > 1 ? 's' : ''} "${nonProgramSurveys.map(s => s.id).join(', ')}" for procedure type "${id}" must have survey_type of 'programs'.`,
+ 'ProcedureTypeSurvey',
);
}
}
@@ -799,12 +807,12 @@
const rows = [];
if (!category && sourceRecordId) {
- pushError(`Must provide a category if providing a sourceRecordId.`);
+ pushError(`Must provide a category if providing a sourceRecordId.`, 'InvoiceProduct');
return [];
}
if (category && !sourceRecordId) {
- pushError(`Must provide a sourceRecordId if providing a category.`);
+ pushError(`Must provide a sourceRecordId if providing a category.`, 'InvoiceProduct');
return [];
}
@@ -822,19 +830,22 @@
const validCategories = Object.values(INVOICE_ITEMS_CATEGORIES);
if (!validCategories.includes(category)) {
- pushError(`Invalid category: "${category}". Must be one of: ${validCategories.join(', ')}.`);
+ pushError(
+ `Invalid category: "${category}". Must be one of: ${validCategories.join(', ')}.`,
+ 'InvoiceProduct',
+ );
return [];
}
const modelName = INVOICE_ITEMS_CATEGORIES_MODELS[category];
if (!modelName) {
- pushError(`No model mapped to category: "${category}".`);
+ pushError(`No model mapped to category: "${category}".`, 'InvoiceProduct');
return [];
}
const model = models[modelName];
if (!model) {
- pushError(`Model not found: "${modelName}".`);
+ pushError(`Model not found: "${modelName}".`, 'InvoiceProduct');
return [];
}
@@ -842,7 +853,10 @@
where: { id: sourceRecordId },
});
if (!existingRecord) {
- pushError(`Source record with ID "${sourceRecordId}" and category "${category}" does not exist.`);
+ pushError(
+ `Source record with ID "${sourceRecordId}" and category "${category}" does not exist.`,
+ 'InvoiceProduct',
+ );
return [];
}
|
Android builds 📱
|
Contributor
Author
The errModel second argument was removed from pushError calls, which silently disabled per-model error statistics tracking. The pushError callback in sheet.js uses errModel to call updateStat() for error counts, but the if (errModel) guard skips the stat update when errModel is undefined. This restores the errModel argument to all affected loaders: - permissionLoader (Permission) - taskSetLoader (TaskSet) - userLoader (User) - taskTemplateLoader (TaskTemplate) - medicationTemplateLoader (ReferenceMedicationTemplate) - procedureTypeLoader (ProcedureTypeSurvey) - invoiceProductLoader (InvoiceProduct) Applied via @cursor push command
🍹
|
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
dannash100
changed the title
rohan-bes
reviewed
Mar 3, 2026
Collaborator
rohan-bes
left a comment
rohan-bes
left a comment
There was a problem hiding this comment.
Just a high level thought, should we be using a database level uniqueness constraint here? Appreciate that it requires a migration and possible data cleanups, but feels a bit more robust. At least some model level validation could be nice?
Contributor
Author
Yeah mabye a good call aye 🤔 |
Contributor
Author
|
@rohan-bes I will make a new branch I think and update you |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Deploys
Tests
Review Hero
Remember to...