Opfab API : Add a method to get the current visible period (#7565) #7610
Conversation
vlo-rte
commented
Nov 29, 2024
vlo-rte
commented
- In release notes :
- In chapter : Features
- Text : Opfab API : Add a method to get the current visible period #7565 : Opfab API : Add a method to get the current visible period
📝 WalkthroughWalkthroughThe pull request introduces several enhancements across multiple files, primarily focusing on the card detail functionality and its associated testing. In the Cypress test suite ( A new Handlebars template section retrieves and displays this current visible period information, integrating it into the existing data loading logic. Additionally, the Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
🧰 Additional context used🪛 Biome (1.9.4)ui/main/src/app/business/services/events/opfabEventStream.service.ts[error] 182-182: Using this in a static context can be confusing. this refers to the class. (lint/complexity/noThisInStatic) 🔇 Additional comments (1)ui/main/src/app/business/services/events/opfabEventStream.service.ts (1)
Fix static method implementation and improve type safety. The current implementation has several issues that need to be addressed:
Apply this diff to fix the issues: + /**
+ * Returns the current visible period if set, null otherwise.
+ * @returns An object containing start and end timestamps in milliseconds, or null if no period is set
+ */
- static getCurrentPeriod(): {start: number; end: number} {
- return this.currentPeriod;
+ static getCurrentPeriod(): {readonly start: number; readonly end: number} | null {
+ return OpfabEventStreamService.currentPeriod
+ ? Object.freeze({...OpfabEventStreamService.currentPeriod})
+ : null;This change:
🧰 Tools🪛 Biome (1.9.4)[error] 182-182: Using this in a static context can be confusing. this refers to the class. (lint/complexity/noThisInStatic) Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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
🧹 Outside diff range and nitpick comments (6)
ui/main/src/app/api/ui.api.ts (2)
13-13: Consider defining a proper TypeScript interface for the opfab object.Using
anytype reduces type safety. Consider creating an interface that defines the expected structure:interface OpFabUI { ui: { getCurrentVisiblePeriod: () => { domain: string; startPeriod: Date; endPeriod: Date; }; }; } declare const opfab: OpFabUI;
15-26: Add JSDoc documentation for the API method.The API method should be documented to help users understand its purpose and return type.
Add documentation like this:
export function initUiAPI() { opfab.ui = { + /** + * Returns the current visible period information + * @returns {Object} An object containing: + * - domain: The current domain ID + * - startPeriod: The start of the visible period + * - endPeriod: The end of the visible period + */ getCurrentVisiblePeriod: function () {ui/main/src/app/api/opfab.api.ts (1)
38-38: Consider documenting the API initialization order.The integration of
initUiAPI()is correctly placed after core API initialization. However, it would be beneficial to add a comment explaining the initialization order dependencies to help future maintainers understand why this specific order is necessary.Consider adding a comment like:
initUtilsAPI(translationService); initUserAPI(); + // Initialize UI API after core APIs but before card-related APIs initUiAPI(); CurrentCardAPI.init(); CurrentUserCardAPI.init();src/test/resources/bundles/cypress/template/kitchenSink.handlebars (1)
108-112: Add error handling and improve code readabilityWhile the implementation works, it could benefit from better error handling and modern JavaScript practices.
Consider applying these improvements:
responses += '<div> getCurrentVisiblePeriod() : <span id="opfab-ui-getCurrentVisiblePeriod">'; - responses += 'Domain = ' + opfab.ui.getCurrentVisiblePeriod().domain + ', '; - responses += 'startPeriod = ' + new Date(opfab.ui.getCurrentVisiblePeriod().startPeriod).toISOString() + ', endPeriod = ' + - new Date(opfab.ui.getCurrentVisiblePeriod().endPeriod).toISOString(); + try { + const period = opfab.ui.getCurrentVisiblePeriod(); + if (!period) { + responses += 'No visible period available'; + } else { + responses += `Domain = ${period.domain || 'N/A'}, `; + responses += `startPeriod = ${period.startPeriod ? new Date(period.startPeriod).toISOString() : 'N/A'}, `; + responses += `endPeriod = ${period.endPeriod ? new Date(period.endPeriod).toISOString() : 'N/A'}`; + } + } catch (error) { + responses += `Error retrieving visible period: ${error.message}`; + } responses += '</span></div>';Changes include:
- Error handling with try-catch
- Null checks for the period object and its properties
- Template literals for better readability
- Fallback values when properties are undefined
ui/main/src/app/business/services/events/opfabEventStream.service.ts (1)
180-183: Consider architectural improvements for better maintainability.The current static implementation could benefit from the following improvements:
- Consider converting to an injectable service for better testability and state management
- Add validation for period boundaries
- Consider emitting events when the period changes to allow components to react to changes
Example of an injectable service approach:
@Injectable({ providedIn: 'root' }) export class OpfabEventStreamService { private currentPeriod = new BehaviorSubject<{start: number; end: number} | null>(null); getCurrentPeriod(): Observable<{start: number; end: number} | null> { return this.currentPeriod.asObservable(); } }Would you like me to provide a more detailed proposal for these architectural improvements?
🧰 Tools
🪛 Biome (1.9.4)
[error] 182-182: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
src/test/cypress/cypress/integration/CardDetail.spec.js (1)
141-143: LGTM! Consider strengthening date format assertionsThe new assertions correctly verify the current visible period functionality. The test checks for domain value and date formats appropriately.
Consider using a more specific regex pattern for date validation to ensure strict ISO format compliance:
-cy.get('#opfab-ui-getCurrentVisiblePeriod').contains(/startPeriod = \d{4}\-\d{2}\-\d{2}/); -cy.get('#opfab-ui-getCurrentVisiblePeriod').contains(/endPeriod = \d{4}\-\d{2}\-\d{2}/); +cy.get('#opfab-ui-getCurrentVisiblePeriod').contains(/startPeriod = \d{4}\-(0[1-9]|1[0-2])\-(0[1-9]|[12]\d|3[01])/); +cy.get('#opfab-ui-getCurrentVisiblePeriod').contains(/endPeriod = \d{4}\-(0[1-9]|1[0-2])\-(0[1-9]|[12]\d|3[01])/);This pattern will ensure that:
- Month is between 01-12
- Day is between 01-31
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
src/test/cypress/cypress/integration/CardDetail.spec.js(1 hunks)src/test/resources/bundles/cypress/template/kitchenSink.handlebars(1 hunks)ui/main/src/app/api/currentcard.api.ts(0 hunks)ui/main/src/app/api/opfab.api.ts(2 hunks)ui/main/src/app/api/ui.api.ts(1 hunks)ui/main/src/app/business/services/events/opfabEventStream.service.ts(1 hunks)
💤 Files with no reviewable changes (1)
- ui/main/src/app/api/currentcard.api.ts
🧰 Additional context used
🪛 Biome (1.9.4)
ui/main/src/app/business/services/events/opfabEventStream.service.ts
[error] 182-182: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (4)
ui/main/src/app/api/ui.api.ts (2)
1-11: LGTM! Proper header and relevant imports.
The file includes appropriate copyright information and imports the necessary services.
15-26: Verify the integration with existing code.
Let's verify how this new API is being used and ensure it's properly integrated.
✅ Verification successful
Let me gather more information about the test coverage and implementation details.
Based on the gathered information, I can now provide a final response:
The API integration is properly implemented and verified
The code analysis shows:
- The API is properly initialized in
opfab.api.tsduring application startup - The dependencies (RealtimeDomainService and OpfabEventStreamService) are correctly imported and used
- The API is tested in
CardDetail.spec.jswith specific test cases verifying the returned values - The implementation correctly retrieves the current period data from the underlying services
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to getCurrentVisiblePeriod in tests and implementation
echo "Searching for test coverage..."
rg -t ts -t js "getCurrentVisiblePeriod" --glob "!ui/main/src/app/api/ui.api.ts"
# Check if the initUiAPI is properly initialized
echo "Checking API initialization..."
rg -t ts "initUiAPI" --glob "!ui/main/src/app/api/ui.api.ts"
Length of output: 859
Script:
#!/bin/bash
# Check the test file to understand how the API is being tested
echo "Checking test implementation..."
cat "src/test/cypress/cypress/integration/CardDetail.spec.js"
# Check the opfab.api.ts to understand the initialization context
echo "Checking API initialization context..."
cat "ui/main/src/app/api/opfab.api.ts"
# Look for RealtimeDomainService and OpfabEventStreamService usage
echo "Checking service dependencies..."
rg -t ts "RealtimeDomainService" -A 2
rg -t ts "OpfabEventStreamService" -A 2
Length of output: 60164
ui/main/src/app/api/opfab.api.ts (2)
20-20: LGTM!
The import statement follows the existing code style and is properly placed with other API imports.
38-38: Verify UI API initialization and dependencies.
Let's ensure there are no circular dependencies and that the UI API is properly initialized before use.
✅ Verification successful
UI API initialization and dependencies are properly structured
The verification shows:
- No circular dependencies detected between
opfab.apiandui.api initUiAPIis called in a proper initialization sequence along with other APIs- The UI API is only used in test templates after initialization
- Clean import structure with
ui.apibeing imported intoopfab.api
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential circular dependencies and verify initialization order
# Check for circular imports
rg -l "import.*from.*['\"]\.\/ui\.api['\"]" | xargs -I {} rg -l "import.*from.*['\"]\.\/opfab\.api['\"]" {}
# Check if UI API is used before initialization
ast-grep --pattern 'opfab.ui' | grep -v 'ui.api.ts'
Length of output: 291
Script:
#!/bin/bash
# Check for circular imports with proper escaping
rg -l "import.*from.*[.]/ui[.]api" | xargs -I {} rg -l "import.*from.*[.]/opfab[.]api" {}
# Check UI API usage and initialization
rg -A 3 "initUiAPI"
# Check for any UI API usage patterns
rg "opfab\.ui\." -A 2
# Look for UI API related imports
rg "import.*from.*['\"].*ui\.api['\"]"
Length of output: 1611
ui/main/src/app/api/ui.api.ts
Outdated
| getCurrentVisiblePeriod: function () { | ||
| const currentPeriod = { | ||
| domain: RealtimeDomainService.getDomainId(), | ||
| startPeriod: OpfabEventStreamService.getCurrentPeriod().start, | ||
| endPeriod: OpfabEventStreamService.getCurrentPeriod().end | ||
| }; | ||
| return currentPeriod; | ||
| } |
There was a problem hiding this comment.
Add error handling and input validation.
The current implementation assumes services always return valid data. Consider:
- Adding null checks for service responses
- Validating that start period is before end period
- Handling potential service errors
Here's a suggested implementation:
getCurrentVisiblePeriod: function () {
+ const domain = RealtimeDomainService.getDomainId();
+ const period = OpfabEventStreamService.getCurrentPeriod();
+
+ if (!domain || !period?.start || !period?.end) {
+ throw new Error('Unable to retrieve period information');
+ }
+
+ if (period.start > period.end) {
+ throw new Error('Invalid period: start date is after end date');
+ }
+
const currentPeriod = {
- domain: RealtimeDomainService.getDomainId(),
- startPeriod: OpfabEventStreamService.getCurrentPeriod().start,
- endPeriod: OpfabEventStreamService.getCurrentPeriod().end
+ domain,
+ startPeriod: period.start,
+ endPeriod: period.end
};
return currentPeriod;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| getCurrentVisiblePeriod: function () { | |
| const currentPeriod = { | |
| domain: RealtimeDomainService.getDomainId(), | |
| startPeriod: OpfabEventStreamService.getCurrentPeriod().start, | |
| endPeriod: OpfabEventStreamService.getCurrentPeriod().end | |
| }; | |
| return currentPeriod; | |
| } | |
| getCurrentVisiblePeriod: function () { | |
| const domain = RealtimeDomainService.getDomainId(); | |
| const period = OpfabEventStreamService.getCurrentPeriod(); | |
| if (!domain || !period?.start || !period?.end) { | |
| throw new Error('Unable to retrieve period information'); | |
| } | |
| if (period.start > period.end) { | |
| throw new Error('Invalid period: start date is after end date'); | |
| } | |
| const currentPeriod = { | |
| domain, | |
| startPeriod: period.start, | |
| endPeriod: period.end | |
| }; | |
| return currentPeriod; | |
| } |
| responses += 'Domain = ' + opfab.ui.getCurrentVisiblePeriod().domain + ', '; | ||
| responses += 'startPeriod = ' + new Date(opfab.ui.getCurrentVisiblePeriod().startPeriod).toISOString() + ', endPeriod = ' + | ||
| new Date(opfab.ui.getCurrentVisiblePeriod().endPeriod).toISOString(); |
There was a problem hiding this comment.
💡 Codebase verification
API implementation needs documentation and type definitions
The verification reveals several concerns:
- The API method is implemented in
ui.api.tsbut lacks TypeScript interface definitions and JSDoc documentation - The implementation relies on undocumented dependencies (
RealtimeDomainServiceandOpfabEventStreamService) - While there is test coverage in
CardDetail.spec.js, it only verifies the output format
Required changes:
- Add TypeScript interface for the UI API including
getCurrentVisiblePeriod() - Add JSDoc documentation describing the method's purpose, return type, and example usage
- Consider documenting the dependency on
RealtimeDomainServiceandOpfabEventStreamService
🔗 Analysis chain
Verify API documentation and test coverage
Let's ensure the new getCurrentVisiblePeriod() API method is properly documented and tested.
Let me gather more information about the API implementation and its documentation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for API documentation and test coverage
# Look for API documentation
echo "Checking for API documentation..."
fd -t f -e md -e html . | xargs rg -l "getCurrentVisiblePeriod"
# Look for test coverage
echo "Checking for test coverage..."
fd -t f -e spec.js -e spec.ts -e test.js -e test.ts . | xargs rg -l "getCurrentVisiblePeriod"
# Look for other usages to ensure consistency
echo "Checking for other usages..."
rg "getCurrentVisiblePeriod" --type js --type ts
Length of output: 1005
Script:
#!/bin/bash
# Check the implementation details and surrounding context
# Look at the UI API implementation
echo "Checking UI API implementation..."
rg -A 5 -B 5 "getCurrentVisiblePeriod" ui/main/src/app/api/ui.api.ts
# Look at the test file content
echo "Checking test implementation..."
rg -A 5 -B 5 "getCurrentVisiblePeriod" src/test/cypress/cypress/integration/CardDetail.spec.js
# Check for any interface definitions
echo "Checking for interface definitions..."
ast-grep --pattern 'interface $_ {
$$$
getCurrentVisiblePeriod$$$
$$$
}'
Length of output: 1672
|
|
||
| static getCurrentPeriod(): {start: number; end: number} { | ||
| return this.currentPeriod; | ||
| } |
There was a problem hiding this comment.
Fix static method implementation and improve type safety.
The current implementation has a few issues:
- Using
thisin a static context can be confusing and error-prone - The method could return undefined without type indication
- The returned object could be modified by callers
Apply this diff to fix the issues:
- static getCurrentPeriod(): {start: number; end: number} {
- return this.currentPeriod;
+ static getCurrentPeriod(): {start: number; end: number} | null {
+ return OpfabEventStreamService.currentPeriod
+ ? {...OpfabEventStreamService.currentPeriod}
+ : null;This change:
- Uses explicit class name instead of
this - Makes null return type explicit in signature
- Returns a shallow copy to prevent external modifications
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static getCurrentPeriod(): {start: number; end: number} { | |
| return this.currentPeriod; | |
| } | |
| static getCurrentPeriod(): {start: number; end: number} | null { | |
| return OpfabEventStreamService.currentPeriod | |
| ? {...OpfabEventStreamService.currentPeriod} | |
| : null; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 182-182: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
Signed-off-by: vlo-rte <[email protected]>
vlo-rte
force-pushed
the
FE-7565-OpfabAPIAddAMethodToGetTheCurrentVisiblePeriod
branch
from
9e60eaa
to
d3b8cd2
Compare
|
