[OSDEV-1930] Implemented displaying additional identifiers such as RBA, LEI, and DUNS IDs on the production location profile page.#594
Merged
vladsha-dev merged 9 commits intomainfrom Apr 23, 2025
Conversation
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
had a problem deploying
to
Quality Environment
GitHub Actions
Failure
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
React App | Jest test suite - Code coverage reportTotal: 33.98%Your code coverage diff: 0.46% ▴ ✅ All code changes are covered |
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Dedupe Hub App | Unittest test suite - Code coverage reportTotal: 56.14%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Contricleaner App | Unittest test suite - Code coverage reportTotal: 98.91%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 |
…-location-profile-page-but-only-for-the-rba-instance
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
…-location-profile-page-but-only-for-the-rba-instance
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
📝 Walkthrough## Walkthrough
This update introduces a new feature flag, `show_additional_identifiers`, managed via a Django migration and surfaced in the front-end through Redux-powered feature flagging. The front-end now conditionally displays additional facility identifiers (DUNS ID, LEI ID, RBA ID) on the production location profile page when the flag is enabled. Related constants and prop types are updated to support the new flag and fields. Several UI components are refactored to use the modern CSS property `overflow-wrap` instead of the deprecated `word-wrap` for better text handling. Comprehensive tests verify the conditional rendering logic.
## Changes
| File(s) | Change Summary |
|----------------------------------------------------------------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `doc/release/RELEASE-NOTES.md` | Updated release notes for version 2.3.0 to document the new database migration, feature flag, front-end changes for additional identifiers, and CSS bugfixes. Removed previously empty sections for clarity. |
| `src/django/api/migrations/0168_introduce_show_additional_identifiers_switch.py` | Added a Django migration to create and remove the `show_additional_identifiers` feature flag using the Waffle `Switch` model. |
| `src/react/src/components/ErrorBoundary.jsx` <br> `src/react/src/components/FacilityDetailsContributorsDrawer.jsx` | Changed CSS from `wordWrap: 'break-word'` to `overflowWrap: 'break-word'` for improved compatibility and standards compliance. |
| `src/react/src/components/FacilityDetailsDetail.jsx` | Changed CSS from `wordWrap: 'break-word'` to `overflowWrap: 'anywhere'` for better text overflow handling in `primaryText` and `secondaryText` classes. |
| `src/react/src/components/FacilityDetailsGeneralFields.jsx` | Refactored to conditionally render additional identifier fields (DUNS ID, LEI ID, RBA ID) based on the `SHOW_ADDITIONAL_IDENTIFIERS` feature flag. Added a helper function for extended field rendering. |
| `src/react/src/util/constants.jsx` | Added `SHOW_ADDITIONAL_IDENTIFIERS` flag, appended new identifier field types to `EXTENDED_FIELD_TYPES`, and introduced the `ADDITIONAL_IDENTIFIERS` array. |
| `src/react/src/util/propTypes.js` | Updated `featureFlagPropType` to include `SHOW_ADDITIONAL_IDENTIFIERS` as a valid flag value. |
| `src/react/src/__tests__/components/FacilityDetailsGeneralFields.test.js` | Added tests for conditional rendering of additional identifier fields in the `FacilityDetailsGeneralFields` component, verifying behavior with the feature flag enabled and disabled. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant Frontend
participant ReduxStore
participant Backend (API)
participant DB (Waffle Switch)
User->>Frontend: Request Facility Profile Page
Frontend->>Backend (API): Fetch facility details & feature flags
Backend (API)->>DB (Waffle Switch): Query 'show_additional_identifiers' flag
DB (Waffle Switch)-->>Backend (API): Return flag state
Backend (API)-->>Frontend: Return facility data & flag state
Frontend->>ReduxStore: Store flag state
Frontend->>Frontend: Conditionally render additional identifiers (DUNS, LEI, RBA) if flag enabled
User-->>Frontend: View profile page with/without additional identifiersPossibly related PRs
Suggested reviewers
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/react/src/components/FacilityDetailsContributorsDrawer.jsx (1)
44-44: Consider using consistent text-wrapping behavior.While modernizing the CSS property from
wordWraptooverflowWrapis good, I notice you're using'break-word'here but'anywhere'in theFacilityDetailsDetailcomponent. Consider using the same value consistently across components for predictable text wrapping behavior.- overflowWrap: 'break-word', + overflowWrap: 'anywhere',src/react/src/__tests__/components/FacilityDetailsGeneralFields.test.js (2)
140-142: Consider enhancing the formatExtendedField mock for more realistic testing.Your current formatter mock is very simple and doesn't reflect how the real component might handle complex data structures. Consider expanding it to better simulate the actual formatter behavior, especially for handling arrays of values.
- const handleFormat = ({ value, formatValue }) => ({ - primary: formatValue(value), - }); + const handleFormat = ({ value, formatValue }) => { + // Handle potential array of values or complex structures + const formattedValue = Array.isArray(value) + ? value.map(v => formatValue(v.value ? v.value.raw_value : v)).join(', ') + : formatValue(value.value ? value.value.raw_value : value); + + return { + primary: formattedValue, + }; + };
156-181: Enhance test coverage with edge cases.Your tests only cover the basic cases of feature flag being true or false. Consider adding tests for edge cases such as:
- What happens if an identifier has multiple entries?
- What happens if an identifier is missing or has null values?
- What happens if the feature flag is undefined?
For example, you could add a test like this:
test('handles missing identifier values gracefully', () => { // Create a modified mock with missing identifier values const modifiedMock = { ...mockData, properties: { ...mockData.properties, extended_fields: { ...mockData.properties.extended_fields, duns_id: [], lei_id: null, rba_id: [{ value: null }] } } }; const preloadedState = { featureFlags: { fetching: false, flags: { show_additional_identifiers: true, }, }, }; const { getByText, queryByText } = renderWithProviders( <FacilityDetailsGeneralFields {...defaultProps} data={modifiedMock} />, { preloadedState }, ); // The labels should still appear but not the values expect(getByText('DUNS ID')).toBeInTheDocument(); expect(queryByText('2120383532')).not.toBeInTheDocument(); // Add similar assertions for other identifiers });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
doc/release/RELEASE-NOTES.md(2 hunks)src/django/api/migrations/0168_introduce_show_additional_identifiers_switch.py(1 hunks)src/react/src/__tests__/components/FacilityDetailsGeneralFields.test.js(1 hunks)src/react/src/components/ErrorBoundary.jsx(1 hunks)src/react/src/components/FacilityDetailsContributorsDrawer.jsx(1 hunks)src/react/src/components/FacilityDetailsDetail.jsx(1 hunks)src/react/src/components/FacilityDetailsGeneralFields.jsx(3 hunks)src/react/src/util/constants.jsx(2 hunks)src/react/src/util/propTypes.js(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/react/src/components/FacilityDetailsDetail.jsx (1)
src/react/src/App.jsx (1)
theme(32-78)
src/react/src/util/propTypes.js (1)
src/react/src/util/constants.jsx (2)
SHOW_ADDITIONAL_IDENTIFIERS(564-564)SHOW_ADDITIONAL_IDENTIFIERS(564-564)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-django-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: run-dd-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: get-base-branch-django-cov
🔇 Additional comments (16)
src/django/api/migrations/0168_introduce_show_additional_identifiers_switch.py (1)
7-14: LGTM: Well-structured migration for feature flag creation.The migration properly creates a Waffle Switch for the new
show_additional_identifiersfeature flag, with a sensible default of inactive. The forward migration creates the switch while the reverse migration correctly removes it, following best practices.src/react/src/util/constants.jsx (3)
564-564: LGTM: New feature flag constant matches migration.The new
SHOW_ADDITIONAL_IDENTIFIERSconstant correctly matches the name defined in the Django migration file, ensuring consistent naming across front-end and back-end.
1077-1091: LGTM: Additional identifiers properly implemented.The three new extended field types for
DUNS ID,LEI ID, andRBA IDfollow the established pattern for defining field types with appropriate formatters.
1094-1098: LGTM: New array for identifier keys.The
ADDITIONAL_IDENTIFIERSfrozen array provides a convenient way to reference all the new identifier field names in one place, making it easier to consistently check for these fields throughout the codebase.src/react/src/components/FacilityDetailsDetail.jsx (2)
23-23: Modernized CSS property with improved text wrapping behavior.The change from deprecated
wordWrapto modernoverflowWrapproperty is good. Changing frombreak-wordtoanywherewill allow text to break at any point (not just word boundaries), which can prevent overflow issues with very long identifiers that might not have spaces.
30-30: Modernized CSS property with consistent behavior.Similar update to the secondary text styling, ensuring consistent text wrapping behavior throughout the component.
src/react/src/components/ErrorBoundary.jsx (1)
13-13: Good CSS update: Replaced deprecated propertyReplaced deprecated
wordWrapCSS property with the modernoverflowWrapproperty. This aligns with best practices and ensures consistent text overflow behavior.src/react/src/util/propTypes.js (2)
33-33: Added new feature flag importProperly imported the
SHOW_ADDITIONAL_IDENTIFIERSconstant from constants file.
360-360: Updated feature flag prop typeAdded the new
SHOW_ADDITIONAL_IDENTIFIERSto the feature flag prop type validation list. This ensures proper type checking for components using this feature flag.src/react/src/components/FacilityDetailsGeneralFields.jsx (4)
16-17: Added necessary imports for feature flag implementationAdded import for
ADDITIONAL_IDENTIFIERSconstant to properly filter extended field types.
19-20: Added feature flag constant importThe
SHOW_ADDITIONAL_IDENTIFIERSconstant is properly imported from constants to enable conditional rendering.
167-182: Well-implemented conditional rendering of extended fieldsGreat implementation of the
renderExtendedFieldsmethod that conditionally shows additional identifiers based on the feature flag. The approach is clean:
- Filter out additional identifiers from extended fields
- Use
FeatureFlagcomponent with appropriatealternativeprop- Render all fields when the flag is enabled or filtered fields when disabled
This ensures that the additional identifiers (DUNS, LEI, RBA IDs) only appear when the backend feature flag is enabled.
206-207: Updated rendering logicProperly replaced direct rendering of extended fields with the new conditional rendering function while maintaining the embed mode logic.
doc/release/RELEASE-NOTES.md (3)
16-16: Good documentation of migrationClearly documented the new migration that introduces the
show_additional_identifiersswitch.
28-29: Well-documented bugfixClear explanation of the CSS property update from
word-wraptooverflow-wrapthat addresses text overflow issues.
32-32: Clear feature documentationWell-documented feature addition explaining how the new additional identifiers will be shown conditionally based on the feature flag.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
doc/release/RELEASE-NOTES.md (2)
29-29: Refine phrasing for clarity
The bugfix entry is accurate, but the phrase “as well as the hash in the React error boundary component” could be more specific. Consider rewording to clarify what “hash” refers to—for example, “the error boundary’s hash display”.
35-35: Improve conditional-rendering description
The “What’s new” bullet is correct, but the clause “once theshow_additional_identifiersfeature flag is returned with a true value from the backend” can be streamlined—for example:
“when the backend returnsshow_additional_identifiers= true.”
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
doc/release/RELEASE-NOTES.md(2 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
doc/release/RELEASE-NOTES.md
31-31: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
32-32: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-fe-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: run-django-code-quality
- GitHub Check: run-dd-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-django-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: get-base-branch-fe-cov
🔇 Additional comments (1)
doc/release/RELEASE-NOTES.md (1)
16-16: New migration documented correctly
The entry for0168_introduce_show_additional_identifiers_switch.pyclearly describes the added feature flag switch. This aligns with the PR’s database migration.
|
Innavin369
approved these changes
Apr 23, 2025
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.
OSDEV-1930
show_additional_identifiersfeature flag is returned with a true value from the backend.show_additional_identifiers, which will be used on the production location profile page to show or hide additional identifiers of the production location.word-wrapCSS property with the supportedoverflow-wrapCSS property.