[GitHub] [ofbiz-framework] PierreSmits opened a new pull request #113: Improved: Convert EntitySyncServices.xml mini-lang to groovy (OFBIZ-11660)

[GitBox]

PierreSmits opened a new pull request #113:
URL: https://github.com/apache/ofbiz-framework/pull/113


   (OFBIZ-11660)
   
   removed: EntitySyncServices.xml
   added: EntitySyncServices.groovy
   modified: services.xml to reflect the change from minilang to groovy for:
   - entitySyncPermissionCheck
   - resetEntitySyncStatus
   modified: webtools/controller.xml to reflect changed request-map and event type regarding reset of EntitySyncStatus
   modified: webtools/widget/EntitySyncForms.xml to reflect changed target regarding reset of EntitySyncStatus


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

sonarcloud[bot] commented on pull request #113:
URL: https://github.com/apache/ofbiz-framework/pull/113#issuecomment-624233550


   Kudos, SonarCloud Quality Gate passed!
   
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_ofbiz-framework&pullRequest=113&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_ofbiz-framework&pullRequest=113&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_ofbiz-framework&pullRequest=113&resolved=false&types=BUG)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_ofbiz-framework&pullRequest=113&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_ofbiz-framework&pullRequest=113&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_ofbiz-framework&pullRequest=113&resolved=false&types=VULNERABILITY) (and [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_ofbiz-framework&pullRequest=113&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/issues?id=apache_ofbiz-framework&pullRequest=113&resolved=false&types=SECURITY_HOTSPOT) to review)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_ofbiz-framework&pullRequest=113&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_ofbiz-framework&pullRequest=113&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_ofbiz-framework&pullRequest=113&resolved=false&types=CODE_SMELL)
   
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/NoCoverageInfo.png' alt='No Coverage information' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_ofbiz-framework&pullRequest=113) No Coverage information  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_ofbiz-framework&pullRequest=113&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_ofbiz-framework&pullRequest=113&metric=new_duplicated_lines_density&view=list)
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

surajkhurana commented on a change in pull request #113:
URL: https://github.com/apache/ofbiz-framework/pull/113#discussion_r420732381



##########
File path: framework/entityext/servicedef/services.xml
##########
@@ -447,7 +447,7 @@ under the License.
         <auto-attributes include="pk" mode="IN" optional="false"/>
     </service>
     <service name="entitySyncPermissionCheck" engine="simple"
-             location="component://entityext/minilang/EntitySyncServices.xml" invoke="entitySyncPermissionCheck">
+             location="component://entityext/groovyScripts/EntitySyncServices.groovy" invoke="entitySyncPermissionCheck">

Review comment:
       IMO, this can be made simpler using:
   
   <service name="entitySyncPermissionCheck" engine="simple"
                location="component://common/minilang/permission/CommonPermissionServices.xml" invoke="genericBasePermissionCheck">
           <implements service="permissionInterface"/>
           <attribute name="primaryPermission" type="String" mode="IN" optional="true" default-value="ENTITY_SYNC"/>
           <attribute name="altPermission" type="String" mode="IN" optional="true"/>
       </service>
   
   After this, you didn't need a groovy method.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

surajkhurana commented on a change in pull request #113:
URL: https://github.com/apache/ofbiz-framework/pull/113#discussion_r420732381



##########
File path: framework/entityext/servicedef/services.xml
##########
@@ -447,7 +447,7 @@ under the License.
         <auto-attributes include="pk" mode="IN" optional="false"/>
     </service>
     <service name="entitySyncPermissionCheck" engine="simple"
-             location="component://entityext/minilang/EntitySyncServices.xml" invoke="entitySyncPermissionCheck">
+             location="component://entityext/groovyScripts/EntitySyncServices.groovy" invoke="entitySyncPermissionCheck">

Review comment:
       IMO, this can be made simpler using:
   
   ```
   <service name="entitySyncPermissionCheck" engine="simple"
                location="component://common/minilang/permission/CommonPermissionServices.xml" invoke="genericBasePermissionCheck">
           <implements service="permissionInterface"/>
           <attribute name="primaryPermission" type="String" mode="IN" optional="true" default-value="ENTITY_SYNC"/>
           <attribute name="altPermission" type="String" mode="IN" optional="true"/>
       </service>
   ```
   
   After this, you didn't need a groovy method.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

PierreSmits commented on a change in pull request #113:
URL: https://github.com/apache/ofbiz-framework/pull/113#discussion_r430054089



##########
File path: framework/entityext/servicedef/services.xml
##########
@@ -447,7 +447,7 @@ under the License.
         <auto-attributes include="pk" mode="IN" optional="false"/>
     </service>
     <service name="entitySyncPermissionCheck" engine="simple"
-             location="component://entityext/minilang/EntitySyncServices.xml" invoke="entitySyncPermissionCheck">
+             location="component://entityext/groovyScripts/EntitySyncServices.groovy" invoke="entitySyncPermissionCheck">

Review comment:
       Thanks for the suggestion, @surajkhurana. I will look into that.
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

danwatford commented on a change in pull request #113:
URL: https://github.com/apache/ofbiz-framework/pull/113#discussion_r641907111



##########
File path: framework/entityext/servicedef/services.xml
##########
@@ -447,7 +447,7 @@ under the License.
         <auto-attributes include="pk" mode="IN" optional="false"/>
     </service>
     <service name="entitySyncPermissionCheck" engine="simple"
-             location="component://entityext/minilang/EntitySyncServices.xml" invoke="entitySyncPermissionCheck">
+             location="component://entityext/groovyScripts/EntitySyncServices.groovy" invoke="entitySyncPermissionCheck">

Review comment:
       Hi @surajkhurana, I was inclined to agree about your suggested change reducing the amount of code in ofbiz and was about to raise a jira issue to refactor other similar permission check services to use the Common Permission Services.
   
   I then realised that doing so would couple the various permission check services to the implementation of the genericBasePermissionCheck, in this case a minilang implementation.
   
   If we later want to re-implement genericBasePermissionCheck using a different engine (groovy, java, etc) then we will need to update all the application specific permission check services that depend on it.
   
   In this case I think it is better to have application specific permission check services depend on the genericBasePermissionCheck service-interface and not the service-implementation.




--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

danwatford commented on a change in pull request #113:
URL: https://github.com/apache/ofbiz-framework/pull/113#discussion_r641980688



##########
File path: framework/entityext/servicedef/services.xml
##########
@@ -447,7 +447,7 @@ under the License.
         <auto-attributes include="pk" mode="IN" optional="false"/>
     </service>
     <service name="entitySyncPermissionCheck" engine="simple"

Review comment:
       Set engine to "groovy"

##########
File path: framework/entityext/servicedef/services.xml
##########
@@ -224,8 +224,8 @@ under the License.
             location="org.apache.ofbiz.entityext.synchronization.EntitySyncServices" invoke="cleanSyncRemoveInfo" auth="true" transaction-timeout="600">
         <description>Clean EntitySyncRemove Info - Generally should be run asynchronously after each sync run, or periodically run on a schedule</description>
     </service>
-    <service name="resetEntitySyncStatusToNotStarted" engine="simple"
-            location="component://entityext/minilang/EntitySyncServices.xml" invoke="resetEntitySyncStatusToNotStarted" auth="true" transaction-timeout="600">
+    <service name="resetEntitySyncStatus" engine="simple"

Review comment:
       engine needs to be set to "groovy"




--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

danwatford closed pull request #113:
URL: https://github.com/apache/ofbiz-framework/pull/113


   


--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

danwatford commented on pull request #113:
URL: https://github.com/apache/ofbiz-framework/pull/113#issuecomment-850954420


   Merged in PR #301


--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]