[ui-Quick Start-Analytics] Updating the analytics Frontend code by integrating with new public API and using predefined hooks #4126
+76
−48
Conversation
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
ananya-agarwal
requested review from
tabraiz12,
JohanAhlen,
bjornalm,
ramprasadagarwal and
Harshg999
|
✅ Test files were modified. Ensure that the tests cover all relevant changes. ✅ |
Python Coverage Report •
Pytest Report
|
bjornalm
reviewed
May 5, 2025
ananya-agarwal
changed the title
ramprasadagarwal
requested changes
May 16, 2025
ananya-agarwal
force-pushed
the
aticks
branch
2 times, most recently
from
5a620d6 to
8d1d537
Compare
There was a problem hiding this comment.
Pull Request Overview
This pull request updates the analytics Frontend by integrating it with a new public API and refactoring the code to use predefined hooks for data loading and saving.
- Updates in test files to use new API endpoints and payload properties.
- Refactoring of the Analytics component to remove local state and use hook-based data management.
- Updates to API utility constants and payload type definitions for consistency with the new API.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| desktop/core/src/desktop/js/apps/admin/Overview/OverviewTab.test.tsx | Updated tests to call new API endpoint and validate payload using analytics_enabled. |
| desktop/core/src/desktop/js/apps/admin/Overview/Analytics.tsx | Refactored Analytics component to use useLoadData and useSaveData hooks and wrapped the UI with a loading/error component. |
| desktop/core/src/desktop/js/apps/admin/Components/utils.ts | Replaced the old API constant with new API endpoints and defined the payload interface. |
Comments suppressed due to low confidence (2)
desktop/core/src/desktop/js/apps/admin/Overview/Analytics.tsx:87
- Consider normalizing the API response to use a single property for the analytics status to reduce ambiguity in the 'checked' prop. If both properties are needed for backward compatibility, please add a comment explaining the rationale.
checked={usageAnalyticsData?.analyticsEnabled ?? usageAnalyticsData?.analytics_enabled ?? false}
desktop/core/src/desktop/js/apps/admin/Overview/Analytics.tsx:90
- Ensure that 'event.target.checked' reliably returns a boolean as expected by the API; consider adding additional type checking if necessary.
onChange={event => updateAnalyticsPreference({ analytics_enabled: event.target.checked })}
0a58cba to
915f373
Compare
UI Coverage Report
|
Python Coverage Report •
Pytest Report
|
bjornalm
approved these changes
Jun 12, 2025
| export const INSTALL_APP_EXAMPLES_API_URL = '/api/v1/install_app_examples'; | ||
| export const INSTALL_AVAILABLE_EXAMPLES_API_URL = '/api/v1/available_app_examples'; | ||
| export const HUE_DOCS_CONFIG_URL = 'https://docs.gethue.com/administrator/configuration/'; | ||
| export const GET_USAGE_ANALYTICS_API_URL = '/api/v1/usage_analytics'; |
There was a problem hiding this comment.
@Harshg999 Why do we have '/api/v1/usage_analytics/update' end point? If we are going for true restful APIs there would be no need for a specific update url right?
bjornalm
reviewed
Jun 12, 2025
| setCollectUsage(newPreference); | ||
| saveCollectUsagePreference(newPreference); | ||
| const handleAnalyticsCheckboxChange = (checked: boolean) => { | ||
| const formData = new FormData(); |
There was a problem hiding this comment.
If we are using the new public APIs why are we still sending formData and not json?
bjornalm
reviewed
Jun 12, 2025
| save: updateAnalyticsPreference, | ||
| loading: updatingAnalyticsPreference, | ||
| error: updateAnalyticsPreferenceError | ||
| } = useSaveData<{ analytics_enabled: boolean }>(UPDATE_USAGE_ANALYTICS_API_URL, { |
There was a problem hiding this comment.
@ramprasadagarwal @ananya-agarwal for useLoadData we get cameCase response but for useSaveData we still get snake_case response, is that correct? And do we have a jira to sync them?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What changes were proposed in this pull request?
Updating the analytics Frontend code by integrating with new public API sent by backend which was updated in the PR:
https://github.com/cloudera/hue/pull/4117/files
How was this patch tested?
The Analytics unit test has also been modified, manual testing
Updated Analytics:
Uploading Screen Recording 2025-06-06 at 3.04.31 PM.mov…
Please review Hue Contributing Guide before opening a pull request.