Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LPD-40961 Indicate and Disable Used Java/Code Data Sets in System Data Set Creation Modal #4650

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

thektan
Copy link

@thektan thektan commented Jan 10, 2025

Story: https://liferay.atlassian.net/browse/LPD-40949
Subtask: https://liferay.atlassian.net/browse/LPD-40961


  • Labels and disables system data sets in the creation modal when a system data set is customized

Mainly wanted to check if this is on the right track:

If this repeated fetching isn't performant, I'm not sure if this possible, but if the System Data Set itself in the SystemFDSEntryRegistry could have a boolean property like customized that could be updated during the ImportSystemDataSetMVCResourceCommand.


Demo

Kapture.2025-01-14.at.18.10.17.mp4

@liferay-continuous-integration
Copy link
Collaborator

CI is automatically triggering the following test suites:

  •     ci:test:sf

@liferay-continuous-integration
Copy link
Collaborator

❌ ci:test:sf - 0 out of 1 jobs passed in 5 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 61b58512ab7f586f7ca6824d9a013acf90aafc3f

Sender Branch:

Branch Name: LPD-40961_IndicateUsedSystemDataSet
Branch GIT ID: 5f01f976754db36da28621e3574ccbf3ed52a8e6

0 out of 1jobs PASSED
For more details click here.
format-source-files:
     [java] java.lang.Exception: Found 1 formatting issues:
     [java] 1: ./modules/apps/frontend-data-set/frontend-data-set-admin-web/src/main/java/com/liferay/frontend/data/set/admin/web/internal/portlet/action/GetSystemDataSetsMVCResourceCommand.java expected:<...opyrightText: (c) 20[25] Liferay, Inc. https...> but was:<...opyrightText: (c) 20[00] Liferay, Inc. https...>
     [java] 
     [java]   at com.liferay.source.formatter.SourceFormatter.format(SourceFormatter.java:472)
     [java]   at com.liferay.source.formatter.SourceFormatter.main(SourceFormatter.java:301)
[stopwatch] [run.batch.test.action: 2:26.314 sec]
     [echo] The following error occurred while executing this line:
     [echo] /opt/dev/projects/github/liferay-portal/portal-impl/build.xml:498: The following error occurred while executing this line:
     [echo] /opt/dev/projects/github/liferay-portal/portal-impl/build.xml:651: Java returned: 1
  [typedef] Could not load definitions from resource org/apache/maven/artifact/ant/antlib.xml. It could not be found.
  [taskdef] Could not load definitions from resource org/jacoco/ant/antlib.xml. It could not be found.
   [delete] Deleting: /opt/dev/projects/github/liferay-portal/null821669788.properties

merge-test-results:
[mkdir] Created dir: /opt/dev/projects/github/liferay-portal/test-results
[beanshell] Truncating errors in /opt/dev/projects/github/liferay-portal/portal-impl/test-results/TEST-JenkinsLogAssertTest.xml
[junitreport] Processing /opt/dev/projects/github/liferay-portal/test-results/TESTS-TestSuites.xml to /tmp/null1256279878
[junitreport] Loading stylesheet ile:/opt/java/ant/lib/ant-junit.jar!/org/apache/tools/ant/taskdefs/optional/junit/xsl/junit-frames.xsl
[junitreport] Transform time: 227ms
[junitreport] Deleting: /tmp/null1256279878
[echo] A report with all the test results can be found at test-results/html/index.html.
[mkdir] Created dir: /opt/dev/projects/github/liferay-jenkins-ee/test-results
[copy] Copying 1 file to /opt/dev/projects/github/liferay-jenkins-ee/test-results
[echo] run.batch.test.tear.down.start.timestamp: 01-09-2025 17:57:03:914 PST
[stopwatch] [run.batch.test.tear.down: 0.000 sec]
[typedef] Could not load definitions from resource org/apache/maven/artifact/ant/antlib.xml. It could not be found.
[taskdef] Could not load definitions from resource org/jacoco/ant/antlib.xml. It could not be found.

@liferay-continuous-integration
Copy link
Collaborator

Jenkins Report:jenkins-report.html
Jenkins Suite:sf
Testray Routine:EE Pull Request
Testray Build ID:106103406

systemFDSEntry.getName(),
dataSetObjectDefinition.
getObjectDefinitionId());

Copy link
Collaborator

@dsanz dsanz Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

System Data Sets come in as OSGi components, so they exist at the entire scope of the server. Nevertheless, object entries do exist on a per virtual instance basis, meaning that in a DXP installation with multiple instances, we'll have system data sets customized in the instance A and yet uncustomized in the instance B.

So, the notion of "used" depends on the system data set name and the company. Because we deploy the object definitions on each company, the object definition we use in this method conveys the right company Id.

In short, we do need to check the object entries in the current company in order to know whether a system data set is being customized or not.

Considering the type of user who will work on this and the usage frequency, I believe this is not causing a significant performance issue at this moment. We could consider improvements in the future.

if (!StringUtil.matchesIgnoreCase(
systemFDSEntry.getTitle(), search)) {

return null;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as we don't implement pagination in this GetSystemDataSetsMVCResourceCommand we don't need to apply search here, it can be a frontend thing.

There's also the possibility of using a DataProvider here, like when we use <fds:classic-display> tag. Such use case is suitable when there is no endpoint to fetch data. Goodies of this is that we get automatic wiring with the sort, search and pagination props coming from the FDS within the data provider getItems() method, in case we need them in the future.

It's true I'm not aware of any mixed usage of a pure-react FDS invocation + a dataprovider implementations. Typically this happens via tag lib but nothing prevents us from adopting this pattern here.

I don't see this as something we must implement now, however, the wiring with

String search = ParamUtil.getString(httpServletRequest, "search");

made me believe this is worth considering.

@markocikos pls chime in, I don't want to overcomplicate this first implementation

Copy link
Author

@thektan thektan Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • After exploring this option of using FDSDataProvider I don’t think this route is ideal, since to follow this pattern we’d most likely have to create
    • The SystemFDSEntriesFDSDataPovider class for the FDSDataProvider service
    • Some sort of SystemFDSEntryManager with a getSystemFDSEntries method that has pagination/keywords (I referenced CETManagerImpl.getCETs())
    • Some similar implementation to FDSApplication.java's getFDSData in the MVCResourceCommand

This is what I found that led me to this conclusion:

  • The implementation in FDSDataProvider doesn’t perform the pagination/sorting/filtering, it is the module-specific-api classes like *LocalService.get*Entries or *Provider.getItems that implements this.
  • From the uses in the taglib, there is a FDSApplication.java in frontend-data-set-taglib that creates an endpoint as the apiURL
    /o/frontend-data-set-taglib/app/data-set/{tableName}/{fdsDataProviderKey} for getting the items (Reference)
    • We could use this endpoint as the apiURL in the FrontendDataSet react component, but since this is taglib-module-specific, I don’t think this same endpoint should be used (unless we moved it outside of the taglib module?)

  • It seems like these classes would be created to simply follow to file structure pattern, since we'd still need to implement all the searching and pagination details ourselves. Keeping the logic contained within the MVCResourceCommand seems more maintainable.
  • However, I did add a change (e1f1afb) to use FDSKeywordsFactory to leverage the default "search" keyword. But I can remove all of this to use frontend search by separately calling fetch and passing the response as items in the FrontendDataSet component.

@thektan thektan force-pushed the LPD-40961_IndicateUsedSystemDataSet branch from 5f01f97 to e1f1afb Compare January 14, 2025 23:52
@thektan
Copy link
Author

thektan commented Jan 15, 2025

Rebased on the updated #4659 and updated the demo video in the PR comment as well

@thektan thektan marked this pull request as ready for review January 15, 2025 02:09
@thektan
Copy link
Author

thektan commented Jan 15, 2025

ci:test:sf

@liferay-continuous-integration
Copy link
Collaborator

❌ ci:test:sf - 0 out of 1 jobs passed in 4 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: ca4c6aaf494430703ef7a971a663855e331de0b8

Sender Branch:

Branch Name: LPD-40961_IndicateUsedSystemDataSet
Branch GIT ID: ccdca3bcd7f7d35f997ed5571370f52296b92f29

0 out of 1jobs PASSED
For more details click here.
format-source-files:
     [java] java.lang.Exception: Found 1 formatting issues:
     [java] 1: ./modules/apps/frontend-data-set/frontend-data-set-admin-web/src/main/java/com/liferay/frontend/data/set/admin/web/internal/portlet/action/GetSystemDataSetsMVCResourceCommand.java expected:<...opyrightText: (c) 20[25] Liferay, Inc. https...> but was:<...opyrightText: (c) 20[00] Liferay, Inc. https...>
     [java] 
     [java]   at com.liferay.source.formatter.SourceFormatter.format(SourceFormatter.java:472)
     [java]   at com.liferay.source.formatter.SourceFormatter.main(SourceFormatter.java:301)
[stopwatch] [run.batch.test.action: 1:46.344 sec]
     [echo] The following error occurred while executing this line:
     [echo] /opt/dev/projects/github/liferay-portal/portal-impl/build.xml:498: The following error occurred while executing this line:
     [echo] /opt/dev/projects/github/liferay-portal/portal-impl/build.xml:651: Java returned: 1
  [typedef] Could not load definitions from resource org/apache/maven/artifact/ant/antlib.xml. It could not be found.
  [taskdef] Could not load definitions from resource org/jacoco/ant/antlib.xml. It could not be found.
   [delete] Deleting: /opt/dev/projects/github/liferay-portal/null142422693.properties

merge-test-results:
[mkdir] Created dir: /opt/dev/projects/github/liferay-portal/test-results
[beanshell] Truncating errors in /opt/dev/projects/github/liferay-portal/portal-impl/test-results/TEST-JenkinsLogAssertTest.xml
[junitreport] Processing /opt/dev/projects/github/liferay-portal/test-results/TESTS-TestSuites.xml to /tmp/null1755935763
[junitreport] Loading stylesheet ile:/opt/java/ant/lib/ant-junit.jar!/org/apache/tools/ant/taskdefs/optional/junit/xsl/junit-frames.xsl
[junitreport] Transform time: 200ms
[junitreport] Deleting: /tmp/null1755935763
[echo] A report with all the test results can be found at test-results/html/index.html.
[mkdir] Created dir: /opt/dev/projects/github/liferay-jenkins-ee/test-results
[copy] Copying 1 file to /opt/dev/projects/github/liferay-jenkins-ee/test-results
[echo] run.batch.test.tear.down.start.timestamp: 01-14-2025 18:15:35:831 PST
[stopwatch] [run.batch.test.tear.down: 0.000 sec]
[typedef] Could not load definitions from resource org/apache/maven/artifact/ant/antlib.xml. It could not be found.
[taskdef] Could not load definitions from resource org/jacoco/ant/antlib.xml. It could not be found.

@liferay-continuous-integration
Copy link
Collaborator

Jenkins Report:jenkins-report.html
Jenkins Suite:sf
Testray Routine:EE Pull Request
Testray Build ID:108882930

@thektan
Copy link
Author

thektan commented Jan 15, 2025

ci:test:sf

@liferay-continuous-integration
Copy link
Collaborator

❌ ci:test:sf - 0 out of 1 jobs passed in 4 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: ca4c6aaf494430703ef7a971a663855e331de0b8

Sender Branch:

Branch Name: LPD-40961_IndicateUsedSystemDataSet
Branch GIT ID: 433d0b68e85f45a58b416866cbb4be93e2c8d1f8

0 out of 1jobs PASSED
For more details click here.
     [exec] > Task :packageRunCheckFormat
     [exec] yarn run v1.13.0
     [exec] \$ node-scripts check:ci
     [exec] 
     [exec] ⚙️ Running preflight checks...
     [exec] 
     [exec] ⚙️ Checking outdated tsconfig.json files ...
     [exec] 
     [exec] ⚙️ Running TypeScript checks on modified files...
     [exec] ℹ️ A total of 12 CPUs were detected: launching tsc using 12 workers
     [exec] ✅ Checked apps/frontend-data-set/frontend-data-set-web
     [exec] ✅ Checked apps/frontend-data-set/frontend-data-set-admin-web
     [exec] 
     [exec] ⚙️ Running format checks on modified files...
     [exec]    /opt/dev/projects/github/liferay-portal/modules/apps/frontend-data-set/frontend-data-set-admin-web/src/main/resources/META-INF/resources/js/SystemDataSets.tsx
     [exec]      7:28  error  'useEffect' is defined but never used. Allowed unused vars must match /^_/u.  @typescript-eslint/no-unused-vars
     [exec]    
     [exec]    ✖ 1 problem (1 error, 0 warnings)
     [exec]    
     [exec]    
     [exec] ❌ CI checks failed.
     [exec] error Command failed with exit code 1.
     [exec] info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
     [exec] 
     [exec] > Task :packageRunCheckFormat FAILED
     [exec] Gradle build finished at 2025-01-15 02:23:12.070.
     [exec] 3 actionable tasks: 3 executed
     [exec] 
     [exec] See the profiling report at: file:///opt/dev/projects/github/liferay-portal/build/reports/profile/profile-2025-01-14-18-22-52.html
     [exec] A fine-grained performance profile is available: use the --scan option.
     [exec] 
     [exec] 
     [exec] FAILURE: Build failed with an exception.
     [exec] 
     [exec] * What went wrong:
     [exec] Execution failed for task ':packageRunCheckFormat'.
     [exec] > Process 'command '/opt/dev/projects/github/liferay-portal/build/node/bin/node'' finished with non-zero exit value 1
     [exec] 
     [exec] * Try:
     [exec] > Run with --info or --debug option to get more log output.
     [exec] > Run with --scan to get full insights.
     [exec] > Get more help at https://help.gradle.org.
     [exec] 
     [exec] * Exception is:
     [exec] org.gradle.api.tasks.TaskExecutionException: Execution failed for task ':packageRunCheckFormat'.
     [exec]   at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.lambda\$executeIfValid\$1(ExecuteActionsTaskExecuter.java:148)

@liferay-continuous-integration
Copy link
Collaborator

Jenkins Report:jenkins-report.html
Jenkins Suite:sf
Testray Routine:EE Pull Request
Testray Build ID:108884705

@thektan thektan force-pushed the LPD-40961_IndicateUsedSystemDataSet branch from 433d0b6 to bff2ec4 Compare January 15, 2025 02:27
@thektan
Copy link
Author

thektan commented Jan 15, 2025

ci:test:sf

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:sf - 1 out of 1 jobs passed in 5 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: ca4c6aaf494430703ef7a971a663855e331de0b8

Sender Branch:

Branch Name: LPD-40961_IndicateUsedSystemDataSet
Branch GIT ID: bff2ec4c6a4bbf35f6aeead01f3d3e3a8d1ce5c3

1 out of 1jobs PASSED
1 Successful Jobs:
For more details click here.

@liferay-continuous-integration
Copy link
Collaborator

Jenkins Report:jenkins-report.html
Jenkins Suite:sf
Testray Routine:EE Pull Request
Testray Build ID:108884733

@thektan
Copy link
Author

thektan commented Jan 15, 2025

ci:test:relevant

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:stable - 24 out of 24 jobs passed

✔️ ci:test:relevant - 30 out of 31 jobs passed in 1 hour 22 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: ca4c6aaf494430703ef7a971a663855e331de0b8

Upstream Comparison:

Branch GIT ID: ca4c6aaf494430703ef7a971a663855e331de0b8
Jenkins Build URL: EE Development Acceptance (master) - 1120 - 2025-01-13[20:36:58]

ci:test:stable - 24 out of 24 jobs PASSED
24 Successful Jobs:
    ci:test:relevant - 29 out of 31 jobs PASSED

    2 Failed Jobs:

    29 Successful Jobs:
      For more details click here.

      This pull contains no unique failures.


      Failures in common with acceptance upstream results at ca4c6aa:
      Test bundle downloads:

      @liferay-continuous-integration
      Copy link
      Collaborator

      @thektan thektan force-pushed the LPD-40961_IndicateUsedSystemDataSet branch from bff2ec4 to 15ea0ff Compare January 15, 2025 23:13
      Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
      Projects
      None yet
      Development

      Successfully merging this pull request may close these issues.

      3 participants