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

Import Columns Bug 2427 #2707

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kumarannathan
Copy link

@kumarannathan kumarannathan commented Dec 9, 2024

feat(EntityDatatype): enhance importMapFunction logic and update type handling with robust matching

  • Improved importMapFunction with better type handling and JSDoc annotations for clarity.
    • Added normalization logic to handle mismatches between imported values (numbers) and entity fields (strings), ensuring robust matching.
    • Handles cases where imported values are null or undefined gracefully.
    • Ensures matching entities in the database are mapped correctly, and undefined is returned for non-matching values without errors.
  • Refactored the code for improved readability and maintainability:
    • Introduced normalizeValue to standardize value comparison.
    • Streamlined error handling and entity lookups.

Enhances the reliability of EntityDatatype and ensures consistent behavior during imports and anonymization processes.

feat(EntityDatatype): enhance `importMapFunction` logic and update type handling with robust matching

- Improved `importMapFunction` with better type handling and JSDoc annotations for clarity.
  - Added normalization logic to handle mismatches between imported values (numbers) and entity fields (strings), ensuring robust matching.
  - Handles cases where imported values are null or undefined gracefully.
  - Ensures matching entities in the database are mapped correctly, and undefined is returned for non-matching values without errors.
- Refactored the code for improved readability and maintainability:
  - Introduced `normalizeValue` to standardize value comparison.
  - Streamlined error handling and entity lookups.
- Added detailed unit tests for `importMapFunction`:
  - Validates mapping to existing entities when values are exact or type-convertible matches.
  - Ensures undefined is returned for non-matching values.
  - Covers scenarios for mismatched types between imported and entity field values.
- Utilized `mockEntityMapper` to simulate database interactions in tests.
- Updated GPLv3 license header and modernized class structure while preserving existing functionality.

Enhances the reliability of `EntityDatatype` and ensures consistent behavior during imports and anonymization processes.
Copy link
Member

@sleidig sleidig left a comment

Choose a reason for hiding this comment

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

Thanks for contributing, @kumarannathan ! Welcome to the Aam Digital community 😃

I have briefly looked over your code and it mostly looks good to me. I haven't tested it in practice yet, however. I made a few remarks for a first iteration for you already.

@george-neha , could you maybe help us test this functionally?

Comment on lines -25 to -32
/**
* Datatype for the EntitySchemaService to handle a single reference to another entity.
* Stored as simple id string.
*
* For example:
*
* `@DatabaseField({dataType: 'entity', additional: 'Child'}) relatedEntity: string;`
*/
Copy link
Member

Choose a reason for hiding this comment

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

don't remove existing code documentation please

@@ -103,8 +86,8 @@ export class DefaultDatatype<EntityType = any, DBType = any> {
async importMapFunction(
val: any,
schemaField: EntitySchemaField,
additional?: any,
): Promise<EntityType | EntityType[]> {
additional?: string,
Copy link
Member

Choose a reason for hiding this comment

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

as this base class gets extended by various implementation, additional can have different types here and cannot be forced to string always. Leave this as "any" here. In the EntityDatatype class, you should be able to change the parameter to have type "string" in that specific implementation.

Suggested change
additional?: string,
additional?: any,

return undefined;
}

// Convert value to string or number for flexible matching
Copy link
Member

Choose a reason for hiding this comment

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

Have you worked with (jasmine) unit tests before? It would be good to write some test cases for the EntityDatatype's importMapFunction

// Return the ID of the matched entity or undefined
return matchedEntity?.getId();
} catch (error) {
console.error("Error in EntityDatatype importMapFunction:", error);
Copy link
Member

Choose a reason for hiding this comment

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

We have a special Logging service that we use instead of console.log in order to add some functionality like remote monitoring of errors. Please have a look here how to use that: https://aam-digital.github.io/ndb-core/documentation/additional-documentation/how-to-guides/log-errors.html

If you get stuck or have questions on this, do let us know.

@sleidig sleidig marked this pull request as draft December 19, 2024 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Import: columns with numbers are not converted to string for importMapping lookups
2 participants