OFBIZ 12174: Improved: Convert InventoryServices.xml mini lang to groovy

[SebastianEcomify]

No description provided.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[danwatford]

I haven't reviewed the code yet, but if converting minilang methods to groovy then the minilang methods should probably be removed under this PR.

[mbrohl]

+1 @danwatford
@SebastianEcomify can you please enhance the PR with the minilang code removed? Thanks!

[danwatford]

Hi @SebastianEcomify and @mbrohl ,

Any objection if other people can take these changes and include them in smaller commits? At the moment it is a bit of an overwhelming task to review the changes to the various service implementations, figure out suitable tests and then approve.

[mbrohl]

@danwatford it would at least be nice to wait for a response before you grab contributions and put them in your own PR's and commit right away. I do not understand why it's easier to split Sebastian's work and create new PR's for it, can you elaborate more where the problem is?

[danwatford]

Hi @mbrohl ,

It certainly wasn't my intention to cause any upset and I am sorry if my actions came across that way.

There is a lot of value locked up in the various PRs that have been submitted to OFBiz which cannot be realised until they are merged. I feel bad for authors who put a lot of time and effort into preparing pull requests which, due to pressures on committers' availability, don't see their contributions merged - particularly when the proposed changes seem in line with the project's goals. (In this case the movement away from simplelang to groovy)

After sending my initial question to you and @SebastianEcomify , I decided to go ahead with partially applying changes - with commit messages crediting Sebastian - since Inventory Management is an area I am actively working with and thought working through the PR would help with my understanding of the area.

I honestly thought you and Sebastian would have been pleased to see someone start working through this PR. If this is not the case please say so, but also please let me know why so that we can void such situations in future.

... I do not understand why it's easier to split Sebastian's work and create new PR's for it, can you elaborate more where the problem is?

The main part of this pull request is a big Groovy script file, porting the implementation more than 30 services from minilang to groovy script. Those services, although notionally related to inventory, implement a variety of functionality.

When I review a PR I try to understand the impact of the change and assess its correctness. In this case that means reading the groovy implementation of each ported service and checking it aligns with the original minilang version. While doing this, I need to ensure I understand both the original and new implementation, and also determine whether any discrepancies are a cause for concern or whether they are just an artifact of two different language approaches.

Case in point: The groovy implementation of updateOldInventoryToDetailSingle sets some entity attributes to null, whereas the original minilang set them to the empty string "". Now there may be no effective difference between approaches, but I still needed to check. (In this case, that checking resulted in OFBIZ-12723 and meant we could remove a bit of code). That checking process should speed up as I become more familiar with minilang.

Next, for each ported service, I need to understand where it is used in OFBiz and identify a way to test that the new implementation works. Since I am not yet familiar with inventory management, this takes a lot of time.

To me, the above takes a lot of work and is difficult to track and communicate progress. Others might find it easy. We are all different.

If I stick with reviewing the original PR, then a lot of my time, as well as the time Sebastian originally spent authoring the PR, won't be realised as value until the entire work is completed, and this probably wouldn't get done.

To make the task of reviewing this PR's changes more management, I decided to try and identify smaller pieces that could be handled as separate PRs, ensuring credit is given to Sebastian for those changes.

In summary, smaller patches are easier to review and test, and less risky to merge.

[danwatford]

Hi @mbrohl and @SebastianEcomify ,

Given my explanation above, are you happy for me to proceed with reviewing and merging Sebastian's work as separate chunks?

[SebastianEcomify]

Hi @danwatford ,
sorry for the late response. I have no objections. You can go ahead and split my work as you see fit.

[PierreSmits]

@danwatford Unfortunately this has attracted some merge-conflicts.