[OSDEV-2331] Added support for the nested objects, integer type, date, and date-time format display in partner fields (Amfori, Worldly and Ulula integrations)#874
Conversation
Dedupe Hub App | Unittest test suite - Code coverage reportTotal: 55.73%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
Contricleaner App | Unittest test suite - Code coverage reportTotal: 98.75%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
Countries App | Unittest test suite - Code coverage reportTotal: 100%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
Django App | Unittest test suite - Code coverage reportTotal: 81.9%Your code coverage diff: 0.01% ▴ ✅ All code changes are covered |
React App | Jest test suite - Code coverage reportTotal: 40.52%Your code coverage diff: 0.20% ▴ ✅ All code changes are covered |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR restructures React components for rendering partner field schema values, moving components into modular FormatComponents and TypeComponents folders while adding support for nested objects, integers, dates, and date-time properties. Django admin changes restrict PartnerField modification lookups to active records only. Test imports are updated to reflect the new folder structure. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In
`@src/react/src/components/PartnerFields/FormatComponents/DateProperty/utils.js`:
- Around line 4-13: The current try/catch around new Date(dateString) is
ineffective because new Date(...) doesn't throw for invalid input; instead check
the constructed Date for validity (e.g., const date = new Date(dateString); if
(isNaN(date.getTime())) return dateString;) before calling
date.toLocaleDateString(...). Update the function using the dateString variable
and the new Date / toLocaleDateString calls so invalid dates return the original
dateString rather than "Invalid Date", and remove or simplify the useless catch
block.
In
`@src/react/src/components/PartnerFields/FormatComponents/DateTimeProperty/utils.js`:
- Around line 4-15: The current DateTime formatting block uses new
Date(dateTimeString) inside a try/catch but new Date() doesn't throw on invalid
input; update the logic in the DateTime formatter (the block using
dateTimeString and new Date(...)) to validate the parsed date by checking
isNaN(date.getTime()) before calling date.toLocaleDateString; if the parsed date
is invalid return the original dateTimeString, otherwise proceed to format with
the existing toLocaleDateString options (year, month, day, hour, minute).
In
`@src/react/src/components/PartnerFields/FormatComponents/UriReferenceProperty/UriReferenceProperty.jsx`:
- Around line 49-56: The anchor currently uses absoluteUri returned by
getAbsoluteUri and can be undefined when partnerConfigFields.baseUrl is falsy;
update UriReferenceProperty (where absoluteUri is computed) to guard before
rendering: if absoluteUri is truthy render the <a> with href={absoluteUri},
otherwise either render a non-interactive <span> showing displayLinkText or fall
back to using propertyValue as the href (e.g., href={propertyValue}) so the
rendered element is not a misleading link; ensure you reference absoluteUri,
propertyValue, displayLinkText and propertyKey when making the change.
In
`@src/react/src/components/PartnerFields/FormatComponents/UriReferenceProperty/utils.js`:
- Around line 3-6: constructUrlFromPartnerField can throw when value is null or
non-string because value.trim() assumes a string; update the function (and the
similar logic used by getAbsoluteUri) to coerce or guard the input before
trimming, e.g., derive a safeValue via value == null ? '' : String(value) and
then use safeValue.trim() in place of value.trim(), preserving the existing
slash handling and return behavior; ensure both constructUrlFromPartnerField and
the getAbsoluteUri call sites use this safe coercion to avoid TypeError for null
or numeric inputs.
In `@src/react/src/components/PartnerFields/PartnerFieldSchemaValue/utils.jsx`:
- Around line 38-54: getComponentForProperty currently lets a schema with type
'object' fall through to TYPE_COMPONENTS['object'] (which maps to
NestedObjectProperty) even when isNestedObject returned false; update the
type-level lookup in getComponentForProperty so it only uses
TYPE_COMPONENTS[type] when type exists and type !== 'object' (i.e., add an
explicit guard against type === 'object'), leaving nested-object handling to
isNestedObject and otherwise falling back to DEFAULT_COMPONENT; reference
getComponentForProperty, isNestedObject, TYPE_COMPONENTS, FORMAT_COMPONENTS, and
DEFAULT_COMPONENT when making the change.
🧹 Nitpick comments (11)
src/django/api/admin.py (1)
295-324: Avoid misleading warnings when editing inactive system fields.
With the active-only manager, inactive system fields will raiseDoesNotExistand log a warning even though the record exists. Consider suppressing the warning when the record exists but is inactive to avoid log noise.🛠️ Optional guard to skip false warnings
- except PartnerField.DoesNotExist: - logger.warning( - f'Partner field `{self.instance.pk}` not found. ' - 'System field must exist in database.' - ) + except PartnerField.DoesNotExist: + if PartnerField.objects.get_all_including_inactive().filter( + pk=self.instance.pk + ).exists(): + return cleaned_data + logger.warning( + f'Partner field `{self.instance.pk}` not found. ' + 'System field must exist in database.' + )src/react/src/components/PartnerFields/FormatComponents/DateProperty/utils.js (1)
16-19: No defensive check whenvalueis nullish.If
valueisnullorundefined,value[propertyKey]will throw aTypeError. Other similar components in this PR (e.g.,UriProperty,IntegerProperty) accessvalue[propertyKey]the same way, but the caller (renderProperty) should guarantee a valid object. Still worth noting thatformatDatehandlesundefinedgracefully via the falsy check, so the only risk is an explicitnull/undefinedvalueargument.src/react/src/components/PartnerFields/FormatComponents/DateTimeProperty/utils.js (1)
6-6: Nit:toLocaleStringis more semantically appropriate when including time components.
toLocaleDateStringwith time options works in practice, buttoLocaleStringis the conventional method when formatting both date and time. Consider usingdate.toLocaleString('en-US', { ... })instead.src/react/src/components/PartnerFields/PartnerFieldSchemaValue/PartnerFieldSchemaValue.jsx (1)
29-38:valuepropType accepts types that will always render asnull.
isValidValuerejects non-object values, sostring,number, andboolin theoneOfTypewill always result in anullreturn. This is presumably intentional (the parent can pass raw values that get silently ignored), but a brief inline comment would clarify this contract for future maintainers.src/react/src/components/PartnerFields/FormatComponents/DateTimeProperty/DateTimeProperty.jsx (1)
8-23: Consider guarding against missing or null date-time values.Unlike
UriPropertywhich returnsnullwhenpropertyValueis falsy,DateTimePropertyalways renders. Ifvalue[propertyKey]isundefinedornull, this will render an empty or broken output (e.g.,"Title: "with nothing after it, or whateverformatDateTime(undefined)returns).Suggested guard
const DateTimeProperty = ({ propertyKey, value, schemaProperties, classes, }) => { const title = getTitleFromSchema(propertyKey, schemaProperties); const formattedDateTime = getFormattedDateTimeValue(propertyKey, value); + if (!formattedDateTime) { + return null; + } + return ( <div className={classes.container}>src/react/src/components/PartnerFields/FormatComponents/DateProperty/DateProperty.jsx (1)
8-18: Same missing-value guard concern asDateTimeProperty.If
value[propertyKey]is absent,getFormattedDateValuereceivesundefinedand the component renders potentially empty or broken output. Consider adding a null guard consistent with theUriPropertypattern.Suggested guard
const DateProperty = ({ propertyKey, value, schemaProperties, classes }) => { const title = getTitleFromSchema(propertyKey, schemaProperties); const formattedDate = getFormattedDateValue(propertyKey, value); + if (!formattedDate) { + return null; + } + return ( <div className={classes.container}>src/react/src/components/PartnerFields/TypeComponents/NestedObjectProperty/NestedObjectProperty.jsx (1)
9-30: Recursive rendering looks correct; consider a depth guard for safety.The mutual recursion between
NestedObjectProperty→PartnerFieldSchemaValue→ (potentially)NestedObjectPropertyis the right approach for nested objects. In practice, partner schemas should be finite-depth, but a malformed or deeply nested schema could cause a stack overflow.If you want to be defensive, you could thread a
depthprop and cap it. This is optional and can be deferred.src/react/src/components/PartnerFields/FormatComponents/UriReferenceProperty/utils.js (2)
29-46: Seven positional parameters make call-sites fragile.
getDisplayLinkTextaccepts 7 positional arguments, which makes it easy to swap arguments at the call site. Consider using a single options object instead.Example
-export const getDisplayLinkText = ( - baseUrl, - displayText, - linkText, - absoluteUri, - schemaProperties, - propertyValue, - propertyKey, -) => { +export const getDisplayLinkText = ({ + baseUrl, + displayText, + linkText, + absoluteUri, + schemaProperties, + propertyValue, + propertyKey, +}) => {
1-6: Minor:lodash/endsWithis unnecessary for a simple string check.Native
String.prototype.endsWithis available in all modern JS environments and avoids pulling in a lodash module for a single check.Proposed simplification
-import endsWith from 'lodash/endsWith'; - -const constructUrlFromPartnerField = (baseUrl, value = '') => { - if (endsWith(baseUrl, '/')) return baseUrl + value.trim(); +const constructUrlFromPartnerField = (baseUrl, value = '') => { + if (baseUrl.endsWith('/')) return baseUrl + value.trim(); return `${baseUrl}/${value.trim()}`; };src/react/src/components/PartnerFields/PartnerFieldSchemaValue/utils.jsx (2)
13-29: The_textsuffix skip logic is tightly coupled to URI format knowledge.
shouldSkipPropertyknows that_text-suffixed keys should be skipped when the base key has a URI format. This works, but the coupling could be surprising to future maintainers. A brief inline comment explaining why would help.Suggested comment
if (propertyKey.endsWith('_text')) { + // URI properties render their own display text, + // so skip companion `*_text` fields to avoid duplication. const baseKey = propertyKey.slice(0, -5);
77-85:isValidValuerejects falsyvalue— fine for objects, but the name could mislead.
!valueon line 78 rejectsnull,undefined,0,'', etc. Since this is only called with object-typed data this is fine, but the generic nameisValidValuemight tempt future callers to use it for scalars. A more descriptive name likeisValidObjectValuewould make the contract clearer.
src/react/src/components/PartnerFields/FormatComponents/DateProperty/utils.js
Outdated
Show resolved
Hide resolved
src/react/src/components/PartnerFields/FormatComponents/DateTimeProperty/utils.js
Outdated
Show resolved
Hide resolved
.../src/components/PartnerFields/FormatComponents/UriReferenceProperty/UriReferenceProperty.jsx
Show resolved
Hide resolved
src/react/src/components/PartnerFields/FormatComponents/UriReferenceProperty/utils.js
Show resolved
Hide resolved
src/react/src/components/PartnerFields/PartnerFieldSchemaValue/utils.jsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/react/src/components/PartnerFields/TypeComponents/NestedObjectProperty/NestedObjectProperty.jsx`:
- Line 17: The displayPropertyKey computation uses propertyKey.replace('_', ' ')
which only replaces the first underscore; update the expression that defines
displayPropertyKey (the variable in NestedObjectProperty.jsx) to replace all
underscores — e.g., use a global regex or split/join on '_' before calling
toUpperCase — so keys like "some_nested_property" become "SOME NESTED PROPERTY".
...ct/src/components/PartnerFields/TypeComponents/NestedObjectProperty/NestedObjectProperty.jsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/react/src/components/PartnerFields/TypeComponents/NestedObjectProperty/NestedObjectProperty.jsx`:
- Line 18: The component reads const nestedValue = value[propertyKey] without
guarding for value being null/undefined or nestedValue not being an object;
update NestedObjectProperty (where nestedValue, value and propertyKey are used)
to check that value is an object and that nestedValue exists and is an object
(or otherwise valid) before rendering PartnerFieldSchemaValue, and return null
or a fallback early if the guard fails so you don't rely solely on
PartnerFieldSchemaValue.isValidValue to reject invalid input.
🧹 Nitpick comments (1)
src/react/src/components/PartnerFields/PartnerFieldSchemaValue/utils.jsx (1)
89-108: Two minor observations onrenderProperties.
Redundant truthiness check (line 104):
renderPropertyalways returns a React element (a truthy object), soif (rendered)is dead code. Consider removing the guard or, if you want to keep it as defensive coding, add a comment explaining why.No null guard on
value:Object.keys(value)will throw aTypeErrorifvalueisnullorundefined. The caller (PartnerFieldSchemaValue) presumably callsisValidValuefirst, butrenderPropertiesis exported publicly and unprotected on its own. An early return (e.g.,if (!value) return [];) would make this safer.Optional hardening
export const renderProperties = ( value, schemaProperties, partnerConfigFields, -) => - Object.keys(value).reduce((acc, propertyKey) => { +) => { + if (!value || typeof value !== 'object') { + return []; + } + + return Object.keys(value).reduce((acc, propertyKey) => { if (shouldSkipProperty(propertyKey, schemaProperties)) { return acc; } const rendered = renderProperty( propertyKey, value, schemaProperties, partnerConfigFields, ); - if (rendered) { - acc.push(rendered); - } + acc.push(rendered); return acc; }, []); +};
...ct/src/components/PartnerFields/TypeComponents/NestedObjectProperty/NestedObjectProperty.jsx
Show resolved
Hide resolved
VadimKovalenkoSNF
left a comment
There was a problem hiding this comment.
Looks clear, approved 👍
protsack-stephan
left a comment
There was a problem hiding this comment.
Great work! Couple of small comments from me.
src/react/src/components/PartnerFields/FormatComponents/DateProperty/utils.js
Outdated
Show resolved
Hide resolved
src/react/src/components/PartnerFields/FormatComponents/DateTimeProperty/utils.js
Outdated
Show resolved
Hide resolved
src/react/src/components/PartnerFields/FormatComponents/UriReferenceProperty/utils.js
Outdated
Show resolved
Hide resolved
src/react/src/components/PartnerFields/PartnerFieldSchemaValue/utils.jsx
Outdated
Show resolved
Hide resolved
src/react/src/components/PartnerFields/PartnerFieldSchemaValue/utils.jsx
Outdated
Show resolved
Hide resolved
src/react/src/components/PartnerFields/TypeComponents/DefaultProperty/utils.js
Outdated
Show resolved
Hide resolved
src/react/src/components/PartnerFields/TypeComponents/NestedObjectProperty/styles.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/react/src/components/PartnerFields/FormatComponents/UriProperty.jsx`:
- Around line 24-31: The anchor is using propertyValue directly for href (see
the JSX anchor with key `${propertyKey}-uri-${propertyValue}`,
href={propertyValue}, linkText) which allows javascript: or data: URIs; update
UriProperty.jsx to validate/sanitize propertyValue before rendering: parse or
check the scheme and only render the <a> when the value begins with a safe
protocol (e.g., startsWith 'http://' or 'https://'), otherwise render linkText
as plain text (or prefix/normalize to https); ensure the same check is applied
where href={propertyValue} is used so no unsafe protocols are emitted to the
DOM.
In
`@src/react/src/components/PartnerFields/FormatComponents/UriReferenceProperty/utils.js`:
- Line 15: The current assignment to stringValue uses a truthy check that drops
valid values like 0 or false; in utils.js update the logic that sets const
stringValue from propertyValue so it treats null/undefined only as empty and
preserves 0/false/'' by using an explicit null/undefined check (e.g., check
propertyValue !== null && propertyValue !== undefined or propertyValue == null
appropriately) and then convert to String(propertyValue) when present; modify
the line that declares stringValue accordingly so integer 0 is retained.
In
`@src/react/src/components/PartnerFields/TypeComponents/DefaultProperty/utils.js`:
- Around line 1-4: The current getPropertyValueAsString function drops valid
falsy values because it uses a truthiness check; update the check to only treat
null/undefined as missing (e.g., test propertyValue === null || propertyValue
=== undefined or the inverse) so that values like 0, false, and '' are preserved
and then return String(propertyValue) when not null/undefined, otherwise return
''. Keep the reference to getPropertyValueAsString and the propertyKey/value
lookup as-is.
In `@src/react/src/components/PartnerFields/utils.js`:
- Around line 18-22: The getFormattedDateValue function currently calls
moment(propertyValue) which can render "Invalid date" for malformed input and
parse date-only strings in local time; update getFormattedDateValue to first
check that propertyValue is non-empty, then create a moment instance using
moment.utc(propertyValue) for date-only values (or otherwise use the existing
parsing strategy if local time is required) and call m.isValid() before
formatting; if !m.isValid() return an empty string (or fallback), and otherwise
return m.format(format). Reference getFormattedDateValue and the local variable
propertyValue when making these changes.
🧹 Nitpick comments (1)
src/react/src/components/PartnerFields/FormatComponents/UriReferenceProperty/utils.js (1)
29-46: Consider an options object to reduce parameter count.This function takes 7 positional parameters, making call sites hard to read and error-prone regarding argument order. A single config/options object would improve clarity. This is a non-blocking suggestion.
src/react/src/components/PartnerFields/FormatComponents/UriReferenceProperty/utils.js
Show resolved
Hide resolved
src/react/src/components/PartnerFields/TypeComponents/DefaultProperty/utils.js
Show resolved
Hide resolved
…isplay-in-partner-fields-amfori-integration
|



OSDEV-2331
<p> </p>) from rich text fields on save, preventing meaningless HTML from being stored in the database.