LPD-9465 - Allow users to save Custom Views #5216
Conversation
|
CI is automatically triggering the following test suites:
|
juanjofgliferay
requested review from
dsanz
and removed request for
liferay-frontend
|
Test suite sf has been triggered on http://test-1-37 |
Summary of ChangesHello @juanjofgliferay, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience by introducing 'Snapshots' for data sets, enabling users to save and manage their personalized view configurations. This includes persisting various display and interaction settings such as visualization modes, pagination, filters, sorting, and column visibility. The implementation involved a comprehensive overhaul of both backend and frontend components, migrating from a preference-based storage system to a more robust Object Entry API for custom views, and integrating a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring of the "Custom Views" feature, now termed "Snapshots". The core change is migrating the storage mechanism from Portal Preferences to a more robust Liferay Object-based solution, which is a great architectural improvement. The changes are comprehensive, spanning the backend, frontend, and tests. Key highlights include the new DataSetSnapshot object, new Vulcan REST endpoints, and a refactored frontend with improved state management using a new deepClone utility for immutability. The PR also includes valuable bug fixes for client extension loading and filter state management, and is accompanied by thorough new Playwright tests. The overall quality of the implementation is high. I have one minor suggestion to improve the serialization of snapshot data.
| ).put( | ||
| "erc", objectEntry.getExternalReferenceCode() | ||
| ).put( | ||
| "label", String.valueOf(properties.get("label")) |
There was a problem hiding this comment.
Using String.valueOf() on a potentially null object will result in the string literal "null". This is likely not what the frontend expects. It's better to pass the raw object from properties.get("label") to JSONUtil.put(), which will correctly serialize a null value to JSON null.
| "label", String.valueOf(properties.get("label")) | |
| "label", properties.get("label") |
There was a problem hiding this comment.
🤔 Not sure. I'm using the same pattern used in other FDS places
liferay-continuous-integration
added
ci:test:sf - success
and removed
ci:test:sf - pending
labels
✔️ ci:test:sf - 1 out of 1 jobs passed in 5 minutesRan com.liferay.source.formatter at released version 1.0.1554. Click here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPD-9465.v2 1 Successful Jobs:For more details click here. |
|
Jenkins Build:test-portal-source-format#14273 Jenkins Report:jenkins-report.html Jenkins Suite:sf Pull Request:liferay-frontend#5216 Testray Routine:EE Pull Request Testray Build:[master] ci:test:sf - juanjofgliferay > liferay-frontend - PR#5216 - 2025-12-05[04:43:55] Testray Build ID: 383827777Testray Importer:test-portal-source-format#14273 |
|
ci:test:frontend-data-set |
|
Test suite frontend-data-set has been triggered on http://test-1-40 |
dsanz
left a comment
dsanz
left a comment
There was a problem hiding this comment.
LGTM ;) added some inline comments for our future selves though
| const tableView = views.find((view) => view.name === 'customizedTable')!; | ||
| const tableView = views.find((view) => | ||
| view.name?.toLowerCase().includes('table') | ||
| )!; |
| <type>com.liferay.frontend.taglib.clay.servlet.taglib.util.CreationMenu</type> | ||
| </attribute> | ||
| <attribute> | ||
| <description>Deprecated as of 7.4.0, replaced by the attribute <![CDATA[<code>snapshotsEnabled</code>]]>.</description> |
There was a problem hiding this comment.
We don't need to mark this as a breaking change, because old "custom views" feature has not been announced to the public
There was a problem hiding this comment.
Ok. I'll change
| settings: any; | ||
| }) { | ||
| if (!appURL) { | ||
| if (Liferay.FeatureFlags['LPD-10683'] || !appURL) { |
There was a problem hiding this comment.
did we end up blocking active view settings reading too?
| [activeCustomViewId]: customView, | ||
| }, | ||
| snapshots: [...updatedSnapshots], | ||
| viewUpdated: false, |
There was a problem hiding this comment.
Minor nit for a followup: this shall be called snapshotUpdated in line with the new naming,
|
|
||
| const fieldAttributes = modifiedFields[name] ?? {}; | ||
|
|
||
| if (!defaultSnapshot.modifiedFields[name]) { |
There was a problem hiding this comment.
can we always count on indexing in defaultSnapshot.modifiedFields ?
There was a problem hiding this comment.
I think so.
| let initialVisibleFieldNames = {}; | ||
|
|
||
| if (activeViewSettings) { | ||
| if (!Liferay.FeatureFlags['LPD-10683'] && activeViewSettings) { |
There was a problem hiding this comment.
I answer myself: yes we did!
| ); | ||
|
|
||
| useEffect(() => { | ||
| const filterClientExtensionDefinitions = filters |
There was a problem hiding this comment.
using filters instead of initialFilters makes a huge difference as now we are:
- chaining the changes one by one, rather than changing
initialFilters(or even worse, mutating its contents) in 2 differenc places. - each change returns a brand-new, deep copy of the filters array
| (!filterClientExtensionDefinitions.length && | ||
| !cellClientExtensionDefinitions.length) || | ||
| cellClientExtensionsLoaded || | ||
| filterClientExtensionsLoaded |
There was a problem hiding this comment.
cellClientExtensionsLoaded || filterClientExtensionsLoaded is a key piece here too, because we don't want to call loadClientExtensions twice.
This can happen now, because initialFilters is no longer an effect dependency but filters is.
| }, [ | ||
| cellClientExtensionsLoaded, | ||
| filterClientExtensionsLoaded, | ||
| filters, |
There was a problem hiding this comment.
Here we are executing the efect everytime we change filters, hence the enhanced gate conditions above to ensure loadClientExtensions call (and the change it entails on the array of filters) just happens once
| onOpenChange={setInfoPanelOpen} | ||
| open={infoPanelOpen} | ||
| {filterClientExtensionsLoading || | ||
| cellClientExtensionsLoading ? ( |
There was a problem hiding this comment.
in the end, do we need to gate managementBar component render to this condition as well?
There was a problem hiding this comment.
No need. It works fine like this.
liferay-continuous-integration
added
ci:test:frontend-data-set - failure
and removed
ci:test:frontend-data-set - pending
labels
❌ ci:test:frontend-data-set - 4 out of 6 jobs passed in 1 hour 17 minutesClick here for more details.Base Branch:Branch Name: master Upstream Comparison:Branch GIT ID: 0c4b7b90f18c43cbbc2dad3967b7f4953e1d5f21 ci:test:frontend-data-set - 4 out of 6 jobs PASSED2 Failed Jobs:
4 Successful Jobs:For more details click here.Failures unique to this pull:For upstream results, click here.Test bundle downloads: |
|
Jenkins Build:test-portal-acceptance-pullrequest(master)#17132 Jenkins Report:jenkins-report.html Jenkins Suite:frontend-data-set Pull Request:liferay-frontend#5216 Testray Routine:EE Pull Request Testray Build:[master] ci:test:frontend-data-set - juanjofgliferay > liferay-frontend - PR#5216 - 2025-12-05[04:49:45] Testray Build ID: 383864809Testray Importer:test-portal-acceptance-pullrequest(master)#17132 |
|
ci:test:sf |
|
Test suite sf has been triggered on http://test-1-40 |
✔️ ci:test:sf - 1 out of 1 jobs passed in 5 minutesRan com.liferay.source.formatter at released version 1.0.1554. Click here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPD-9465.v2 1 Successful Jobs:For more details click here. |
|
Jenkins Build:test-portal-source-format#9556 Jenkins Report:jenkins-report.html Jenkins Suite:sf Pull Request:liferay-frontend#5216 Testray Routine:EE Pull Request Testray Build:[master] ci:test:sf - juanjofgliferay > liferay-frontend - PR#5216 - 2025-12-05[10:11:35] Testray Build ID: 384077935Testray Importer:test-portal-source-format#9556 |
|
ci:test:sf |
|
ci:forward |
|
Test suite relevant has been triggered on http://test-1-40 |
|
CI is automatically triggering the following test suites:
The pull request will automatically be forwarded to the user
|
|
Skipping previously passed test suites:
|
Build started.Jenkins is currently running tests in the Base Branch:Branch Name: master Job Summary:Job Link: test-portal-acceptance-pullrequest(master) For more details click here. |
|
ci:stop |
|
ci:forward:force |
|
ci:test:sf |
2 similar comments
|
ci:test:sf |
|
ci:test:sf |
|
Test suite sf has been triggered on http://test-1-31 |
✔️ ci:test:sf - 1 out of 1 jobs passed in 6 minutesRan com.liferay.source.formatter at released version 1.0.1554. Click here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPD-9465.v2 1 Successful Jobs:For more details click here. |
|
Jenkins Build:test-portal-source-format#8634 Jenkins Report:jenkins-report.html Jenkins Suite:sf Pull Request:liferay-frontend#5216 Testray Routine:EE Pull Request Testray Build:[master] ci:test:sf - juanjofgliferay > liferay-frontend - PR#5216 - 2025-12-10[07:19:33] Testray Build ID: 386545803Testray Importer:test-portal-source-format#8634 |
|
ci:forward:force |
|
Test suite relevant has been triggered on http://test-1-31 |
|
CI is automatically triggering the following test suites:
The pull request will automatically be forwarded to the user
|
|
Skipping previously completed test suites:
|
Build started.Jenkins is currently running tests in the Base Branch:Branch Name: master Job Summary:Job Link: test-portal-acceptance-pullrequest(master) For more details click here. |
3 participants
Followup #5175
Story / Task
LPD-9465 / LPD-59387
Warning
In the FDS Sample Custom Views, Config in URL and Active View Settings may clash and return unexpected results.
If you want to test Custom/User Views in the FDS Sample it is better to start with a fresh DB or remove the corresponding
ActiveViewSettingsJSONrows from thePortalPreferenceValuetable.Tip
To test this feature we need to update a given Data Set and set
customViewsEnabledto true. This can be done throug API Explorer using the Data Sets API/by-external-reference-code/{externalReferenceCode}PATCH with{"snapshotsEnabled": true}body.To ease testing, it is possible to enable custom views via UI with
LPD-10683feature flag turned on.Goal
Allows end-users to create, edit and delete customized versions of admin views.
Aspects that can be saved as part of the custom view:
UI
Screencast.From.2025-11-20.13-01-15.mp4
Tests
FDS web
FDS admin web