You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ofbiz.apache.org by GitBox <gi...@apache.org> on 2020/05/05 18:34:25 UTC
[GitHub] [ofbiz-framework] PierreSmits opened a new pull request #113: Improved: Convert EntitySyncServices.xml mini-lang to groovy (OFBIZ-11660)
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:
users@infra.apache.org
[GitHub] [ofbiz-framework] danwatford commented on a change in pull request #113: Improved: Convert EntitySyncServices.xml mini-lang to groovy (OFBIZ-11660)
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [ofbiz-framework] PierreSmits commented on a change in pull request #113: Improved: Convert EntitySyncServices.xml mini-lang to groovy (OFBIZ-11660)
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [ofbiz-framework] surajkhurana commented on a change in pull request #113: Improved: Convert EntitySyncServices.xml mini-lang to groovy (OFBIZ-11660)
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [ofbiz-framework] sonarcloud[bot] commented on pull request #113: Improved: Convert EntitySyncServices.xml mini-lang to groovy (OFBIZ-11660)
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [ofbiz-framework] surajkhurana commented on a change in pull request #113: Improved: Convert EntitySyncServices.xml mini-lang to groovy (OFBIZ-11660)
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [ofbiz-framework] danwatford commented on pull request #113: Improved: Convert EntitySyncServices.xml mini-lang to groovy (OFBIZ-11660)
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [ofbiz-framework] danwatford commented on a change in pull request #113: Improved: Convert EntitySyncServices.xml mini-lang to groovy (OFBIZ-11660)
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [ofbiz-framework] danwatford closed pull request #113: Improved: Convert EntitySyncServices.xml mini-lang to groovy (OFBIZ-11660)
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org