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

ET-889: fix(elation): resolving document #573

Conversation

nckhell
Copy link
Contributor

@nckhell nckhell commented Feb 3, 2025

No description provided.

Copy link

github-actions bot commented Feb 3, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Backward Compatibility

The resolvingDocument field is marked as optional in the validation schema to maintain backward compatibility with existing workflows. Ensure this approach is sufficient and does not introduce unexpected behavior.

// Making it optional as making it required would break the extension for existing workflows
// I'll make it required once we've updated the Suvida flows
resolvingDocument: NumericIdSchema.optional(),
API Integration

The resolving_document field is added to the API payload. Validate that the API client and backend handle this new field correctly without errors.

const { referralOrderId, resolutionState, resolvingDocument } =
  FieldsValidationSchema.parse(payload.fields)
const api = makeAPIClient(payload.settings)

try {
  await api.updateReferralOrder(referralOrderId, {
    resolution: {
      state: resolutionState,
      resolving_document: resolvingDocument,

Copy link

github-actions bot commented Feb 3, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
General
Add stricter validation for optional fields

Ensure that the resolvingDocument field is validated properly when marked as
optional to avoid unexpected runtime errors or inconsistencies in workflows.

extensions/elation/actions/updateReferralOrderResolution/config/fields.ts [46]

-resolvingDocument: NumericIdSchema.optional(),
+resolvingDocument: NumericIdSchema.optional().refine(value => value === undefined || typeof value === 'number', {
+  message: 'resolvingDocument must be a numeric ID or undefined',
+}),
Suggestion importance[1-10]: 9

Why: The suggestion enhances validation for the resolvingDocument field by ensuring it is either undefined or a numeric ID, which reduces the risk of runtime errors and improves data integrity. This is highly relevant given the field's optional nature.

9
Add test cases for edge scenarios

Include test cases to verify behavior when resolvingDocument is undefined or invalid
to ensure robust validation and error handling.

extensions/elation/actions/updateReferralOrderResolution/updateReferralOrderResolution.test.ts [94]

-resolvingDocument: 1234567890,
+// Test case for undefined resolvingDocument
+resolvingDocument: undefined,
Suggestion importance[1-10]: 7

Why: Adding test cases for scenarios where resolvingDocument is undefined or invalid ensures the robustness of the validation and error handling logic. This is a valuable addition to maintain code quality and reliability.

7
Possible issue
Handle undefined values in API calls

Add error handling for cases where resolvingDocument is undefined or invalid to
ensure the API call does not fail unexpectedly.

extensions/elation/actions/updateReferralOrderResolution/updateReferralOrderResolution.ts [29]

-resolving_document: resolvingDocument,
+resolving_document: resolvingDocument || null,
Suggestion importance[1-10]: 8

Why: The suggestion ensures that undefined values for resolvingDocument are handled gracefully in API calls by substituting them with null. This improves robustness and prevents potential API errors.

8

resolvingDocument: {
id: 'resolvingDocument',
label: 'Resolving document',
type: FieldType.STRING,
Copy link
Contributor

Choose a reason for hiding this comment

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

should be FieldType.NUMERIC

@bejoinka bejoinka force-pushed the et-889-check-that-referral-order-is-fulfilled-correctly-in-elation branch from 297dd49 to 1b3f41b Compare February 3, 2025 19:18
@bejoinka bejoinka merged commit 93c4cd2 into main Feb 3, 2025
2 checks passed
@bejoinka bejoinka deleted the et-889-check-that-referral-order-is-fulfilled-correctly-in-elation branch February 3, 2025 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants