Re: svn commit: r1819730 [1/6] - in /ofbiz/ofbiz-framework/trunk/applications: content/src/main/java/org/apache/ofbiz/content/ content/src/main/java/org/apache/ofbiz/content/compdoc/ content/src/main/
Le 01/01/2018 à 13:13, Michael Brohl a écrit : I won't insist on this further. There seems to be a substantial difference in perception how we should handle these issues and I'm tired to discuss this again and again. Hope you will take care about the fixes resulting from the commit if they are necessary. Yes as I said I'll review again. I hope the contributors (Suraj and Anushi ) and others will help me do so, it's indeed a large task I would expect that the testing was done before providing the patches. You cannot rely on the tests in all cases because different return results might not affect the single event/service but the integration logic/flow. I am not sure if we have enough tests for this. The integration tests are/have-been mostly designed with the functional aspect in mind. I don't think we have much tests by service, if any. And yes we have not enough tests at large, but we have already 1307 integration tests and 26 Gradle unit tests Despite them it's really hard to test only. We have almost to decide on each change. For instance should we keep an continue in a previous way or not. Sometimes it's not obvious. See my last (today) comments in OFBIZ-9981 for instance Jacques
Re: svn commit: r1819730 [1/6] - in /ofbiz/ofbiz-framework/trunk/applications: content/src/main/java/org/apache/ofbiz/content/ content/src/main/java/org/apache/ofbiz/content/compdoc/ content/src/main/
We had this discussion too many times. Big mass-changes that are unfocused are a bad idea. We've witnessed many issues in the past and the info is available in other threads. I wish if Jacques would stop these massive lets-go-after-a-pattern-everywhere kind of commits. I leave it for the rest of the community to decide. I am beginning to really get tired from these unending and repeating discussions. On Jan 1, 2018 3:13 PM, "Michael Brohl" wrote: > Inline... > > > Am 01.01.18 um 12:45 schrieb Jacques Le Roux: > >> Le 01/01/2018 à 11:42, Michael Brohl a écrit : >> >>> For me, reviewing is much easier if I can apply a patch to the codebase >>> and use the IDE tools to examine the diff with the code contexts. >>> >> This is something I'll take into account in the future. I'll ask for >> reviews before committing such large patches. >> > That would be a good idea... > >> >> This is not possible for me now the code is already committed. I won't >>> review this in the my email client. >>> >> Actually if you really want to review there is a possibility. >> You copy you current trunk working copy under another name. >> You svn update just before the commit (here at r1819694). >> You modify the name in .project with the copied name. >> You import in the IDE, you patch, it's ready for a review. >> This can be done in minutes, the longer part being the copy. >> > This is unnecessary complicated compared to just applying a patch and I > won't do it also in this case. > >> >> >>> This is a big commit and most of the patches were submitted just before >>> Christmas. I cannot see the urge to commit the work so fast without letting >>> more people review. >>> >> 2 reasons: >> >> 1. Large patches reviews (and not only large ones) tend to be postponed >> (we are humans), then are deprecated and can't be applied so get forgotten. >>I'm sure I can find several examples in Jira. >> 2. When in trunk they can be "hand" and "automatically" (thanks to >> spiders) tested, in demo. Also by people running them on their machine. >> They are >>not supposed to be used in production of course. >> > > My standpoint in these cases is to do a review and dicussion before the > commit. If there is a chance and others do not take it, you can commit > later. That does not postpone the issue too much. > > We should also take into account that people might use the trunk as base > of their work. Even if trunk is not considered stable and we have releases, > we should still try to be as stable as possible in trunk. > > I won't insist on this further. There seems to be a substantial difference >>> in perception how we should handle these issues and I'm tired to discuss >>> this again and again. Hope you will take care about the fixes resulting >>> from the commit if they are necessary. >>> >> Yes as I said I'll review again. I hope the contributors (Suraj and >> Anushi ) and others will help me do so, it's indeed a large task >> > I would expect that the testing was done before providing the patches. You > cannot rely on the tests in all cases because different return results > might not affect the single event/service but the integration logic/flow. I > am not sure if we have enough tests for this. > > Anyway, I made my point clear (now and in the past) and I don't like the > process. It's up to you to take responsibility if things are broken. > I'm finished with this discussion. > > Michael > >> >> Jacques >> >> >>> Michael >>> >>> >>> Am 01.01.18 um 08:53 schrieb Jacques Le Roux: >>> Michael, I'll review again but will not revert. It will take time, but will be done before we create the next release branch. This will allow usage, on demo at least, to help finding possible issues. My basic idea is simple: we should handle error in services like exceptions in Java: no swallowing at all. Handling a service returning an error only with a debug log error should be only done when strictly necessary, ie when we don't want to derail the flow. There are some cases in the commits (plugins is another one). To answer you Taher, no I did not test all services manually. Of course I'd appreciate any help in reviewing... Jacques Le 31/12/2017 à 23:49, Jacques Le Roux a écrit : > It's far better than before. I reviewed all carefully and tests pass. > If you see something specific which hurts you please fix it > > Thanks > > Jacques > > > Le 31/12/2017 à 15:41, Michael Brohl a écrit : > >> I have not the time to look at this further now but some changes seem >> to change the business logic, e.g. by returning an "error" result in >> events >> where they did not before, same in services. It makes a difference in >> both >> the controller logic as well as in the service engine logic. >> >> This seems not to be a simple refactoring and we should have some >> thorough test
Re: svn commit: r1819730 [1/6] - in /ofbiz/ofbiz-framework/trunk/applications: content/src/main/java/org/apache/ofbiz/content/ content/src/main/java/org/apache/ofbiz/content/compdoc/ content/src/main/
Le 01/01/2018 à 12:45, Jacques Le Roux a écrit : 2. When in trunk they can be "hand" and "automatically" (thanks to spiders) tested, in demo. Also by people running them on their machine. They are not supposed to be used in production of course. Of course I'd not do that just before freezing a release. Here we have a year ahead to validate the commit... Jacques
Re: svn commit: r1819730 [1/6] - in /ofbiz/ofbiz-framework/trunk/applications: content/src/main/java/org/apache/ofbiz/content/ content/src/main/java/org/apache/ofbiz/content/compdoc/ content/src/main/
I have not the time to look at this further now but some changes seem to change the business logic, e.g. by returning an "error" result in events where they did not before, same in services. It makes a difference in both the controller logic as well as in the service engine logic. This seems not to be a simple refactoring and we should have some thorough testing before this goes into the codebase. Regards, Michael Am 31.12.17 um 15:11 schrieb Taher Alkhateeb: I don't know what the rest of the team thinks, but I am really worried from this commit. It is one of those mass updates which I constantly warn against. Can you please clarify what kind of testing was done against all the services that were updated and the context of each service? On Sun, Dec 31, 2017 at 2:11 PM, wrote: Author: jleroux Date: Sun Dec 31 11:11:46 2017 New Revision: 1819730 URL: http://svn.apache.org/viewvc?rev=1819730&view=rev Log: Improved: Handle service response effectively (OFBIZ-9981) As per discussion on Dev ML: == Every service calling from java/groovy must handle errors by service util methods such as isError, returnError etc. and similarly in case of XML to make sure service was executed successfully. Apart from this, one suggestion is to include *Debug.logError* in *ServiceUtil.returnProblem* so that in case of any error occurred and handled, it will always be logged on the console. == jleroux: this is the applications part with some slight changes Thanks: Suraj Khurana Modified: ofbiz/ofbiz-framework/trunk/applications/content/src/main/java/org/apache/ofbiz/content/ContentManagementEvents.java ofbiz/ofbiz-framework/trunk/applications/content/src/main/java/org/apache/ofbiz/content/ContentManagementServices.java ofbiz/ofbiz-framework/trunk/applications/content/src/main/java/org/apache/ofbiz/content/ConvertTree.java ofbiz/ofbiz-framework/trunk/applications/content/src/main/java/org/apache/ofbiz/content/compdoc/CompDocEvents.java ofbiz/ofbiz-framework/trunk/applications/content/src/main/java/org/apache/ofbiz/content/content/ContentPermissionServices.java ofbiz/ofbiz-framework/trunk/applications/content/src/main/java/org/apache/ofbiz/content/content/ContentServices.java ofbiz/ofbiz-framework/trunk/applications/content/src/main/java/org/apache/ofbiz/content/content/ContentWorker.java ofbiz/ofbiz-framework/trunk/applications/content/src/main/java/org/apache/ofbiz/content/content/UploadContentAndImage.java ofbiz/ofbiz-framework/trunk/applications/content/src/main/java/org/apache/ofbiz/content/data/DataEvents.java ofbiz/ofbiz-framework/trunk/applications/content/src/main/java/org/apache/ofbiz/content/data/DataResourceWorker.java ofbiz/ofbiz-framework/trunk/applications/content/src/main/java/org/apache/ofbiz/content/layout/LayoutEvents.java ofbiz/ofbiz-framework/trunk/applications/content/src/main/java/org/apache/ofbiz/content/survey/PdfSurveyServices.java ofbiz/ofbiz-framework/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/bom/BOMHelper.java ofbiz/ofbiz-framework/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/bom/BOMNode.java ofbiz/ofbiz-framework/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/bom/BOMServices.java ofbiz/ofbiz-framework/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ProductionRun.java ofbiz/ofbiz-framework/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ProductionRunEvents.java ofbiz/ofbiz-framework/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ProductionRunServices.java ofbiz/ofbiz-framework/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/mrp/MrpServices.java ofbiz/ofbiz-framework/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/mrp/ProposedOrder.java ofbiz/ofbiz-framework/trunk/applications/marketing/src/main/java/org/apache/ofbiz/sfa/vcard/VCard.java ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/OrderManagerEvents.java ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderChangeHelper.java ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderEvents.java ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderLookupServices.java ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReturnServices.java ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderServices.java ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/quote/QuoteServices.
Re: svn commit: r1819730 [1/6] - in /ofbiz/ofbiz-framework/trunk/applications: content/src/main/java/org/apache/ofbiz/content/ content/src/main/java/org/apache/ofbiz/content/compdoc/ content/src/main/
I don't know what the rest of the team thinks, but I am really worried from this commit. It is one of those mass updates which I constantly warn against. Can you please clarify what kind of testing was done against all the services that were updated and the context of each service? On Sun, Dec 31, 2017 at 2:11 PM, wrote: > Author: jleroux > Date: Sun Dec 31 11:11:46 2017 > New Revision: 1819730 > > URL: http://svn.apache.org/viewvc?rev=1819730&view=rev > Log: > Improved: Handle service response effectively > (OFBIZ-9981) > > As per discussion on Dev ML: > == > Every service calling from java/groovy must handle errors by service util > methods such as isError, returnError etc. > and similarly in case of XML > to make sure service was executed successfully. > > Apart from this, one suggestion is to include *Debug.logError* in > *ServiceUtil.returnProblem* so that in case of any error occurred and handled, > it will always be logged on the console. > == > > jleroux: this is the applications part with some slight changes > > Thanks: Suraj Khurana > > Modified: > > ofbiz/ofbiz-framework/trunk/applications/content/src/main/java/org/apache/ofbiz/content/ContentManagementEvents.java > > ofbiz/ofbiz-framework/trunk/applications/content/src/main/java/org/apache/ofbiz/content/ContentManagementServices.java > > ofbiz/ofbiz-framework/trunk/applications/content/src/main/java/org/apache/ofbiz/content/ConvertTree.java > > ofbiz/ofbiz-framework/trunk/applications/content/src/main/java/org/apache/ofbiz/content/compdoc/CompDocEvents.java > > ofbiz/ofbiz-framework/trunk/applications/content/src/main/java/org/apache/ofbiz/content/content/ContentPermissionServices.java > > ofbiz/ofbiz-framework/trunk/applications/content/src/main/java/org/apache/ofbiz/content/content/ContentServices.java > > ofbiz/ofbiz-framework/trunk/applications/content/src/main/java/org/apache/ofbiz/content/content/ContentWorker.java > > ofbiz/ofbiz-framework/trunk/applications/content/src/main/java/org/apache/ofbiz/content/content/UploadContentAndImage.java > > ofbiz/ofbiz-framework/trunk/applications/content/src/main/java/org/apache/ofbiz/content/data/DataEvents.java > > ofbiz/ofbiz-framework/trunk/applications/content/src/main/java/org/apache/ofbiz/content/data/DataResourceWorker.java > > ofbiz/ofbiz-framework/trunk/applications/content/src/main/java/org/apache/ofbiz/content/layout/LayoutEvents.java > > ofbiz/ofbiz-framework/trunk/applications/content/src/main/java/org/apache/ofbiz/content/survey/PdfSurveyServices.java > > ofbiz/ofbiz-framework/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/bom/BOMHelper.java > > ofbiz/ofbiz-framework/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/bom/BOMNode.java > > ofbiz/ofbiz-framework/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/bom/BOMServices.java > > ofbiz/ofbiz-framework/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ProductionRun.java > > ofbiz/ofbiz-framework/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ProductionRunEvents.java > > ofbiz/ofbiz-framework/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ProductionRunServices.java > > ofbiz/ofbiz-framework/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/mrp/MrpServices.java > > ofbiz/ofbiz-framework/trunk/applications/manufacturing/src/main/java/org/apache/ofbiz/manufacturing/mrp/ProposedOrder.java > > ofbiz/ofbiz-framework/trunk/applications/marketing/src/main/java/org/apache/ofbiz/sfa/vcard/VCard.java > > ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/OrderManagerEvents.java > > ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderChangeHelper.java > > ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderEvents.java > > ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderLookupServices.java > > ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReturnServices.java > > ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderServices.java > > ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/quote/QuoteServices.java > > ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/requirement/RequirementServices.java > > ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/shoppingcart/CheckOutEvents.java > > ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz
