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

771 composite order mapper needs to remove invalid characters from po numbers #825

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ealexch
Copy link
Collaborator

@ealexch ealexch commented Jan 14, 2025

Purpose

This PR addresses the issue where PO numbers with invalid characters were not being sanitized properly, potentially causing issues during migration. Fixes #771. The solution involves sanitizing PO numbers by removing invalid characters and logging any potential duplicates after sanitization.

Changes Made in this PR

  • Implemented a sanitize_po_number method to remove invalid characters from PO numbers.
  • Added logic to detect duplicates after sanitization and log them using Helper.log_data_issue.
  • Used a class-level set to track existing PO numbers globally across the migration process.

Code Review Specifics

  • Read the code and suggest improvements, if necessary.
  • Ensure PO numbers with invalid characters are correctly sanitized, and duplicates are logged appropriately.

Warning Checklist

  • New dependencies added.
  • Includes breaking changes.

How to Verify

  1. Run the order migrator with sample data containing PO numbers with invalid characters.
  2. Verify that PO numbers are sanitized correctly and that duplicates are logged as data issues.

Open Questions

None at the moment.

Learn Anything Cool?

I learned about handling data sanitization in migration tools and ensuring data integrity by detecting and logging potential duplicates.

Copy link

codecov bot commented Jan 14, 2025

Codecov Report

Attention: Patch coverage is 60.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 72.18%. Comparing base (07b79a9) to head (526230e).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
..._tools/mapping_file_transformation/order_mapper.py 60.00% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #825      +/-   ##
==========================================
- Coverage   72.19%   72.18%   -0.02%     
==========================================
  Files         104      104              
  Lines       11521    11531      +10     
  Branches     1387     1389       +2     
==========================================
+ Hits         8318     8324       +6     
- Misses       2904     2906       +2     
- Partials      299      301       +2     
Flag Coverage Δ
unittests 72.18% <60.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@bltravis bltravis left a comment

Choose a reason for hiding this comment

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

@ealexch See comment regarding implementing the sanitize method as a static method, and please add a test to cover the new method.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason you didn't implement this as a static method? It doesn't need access to self and it would make it easier to write a test to cover the new code.

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.

Composite order mapper needs to remove invalid characters from PO Numbers
3 participants