I think your contribution is very useful, and the code has a good quality. However, I ask your some modification in order to be coherent in style with the already existing OX code.
These are my requests:
1. Rename LiferayLibraryUtil to LiferayDocumentLibrary: The other classes from org.openxava.util do not use the suffix 'Util'
2. Integrate the methods in ObjectUils in the already existing Objects
3. Use the already existing Emails intestad of MailUtils
4- Remove the references to Liferay code in SimpleHtmlReportAction: You utility can be used from outside of Liferay or from other portals. You can move this code to another classes (a subclass maybe?)
5. Use WebEditors.format instead of FormatUtils.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
1. Done
2. Done
3. Done: I have not looked enough at your code, I should have used this right from the beginning...
4. Done
I have created a new class which extends SimpleHtmlReportAction called SimpleLiferayHtmlReportAction.
The getLiferayLibraryImages allows for very easy production of professional datasheets...
5. I have removed FormatUtils but I did not get the howto for your comment...
I have therefore relied on the DateFormat.getInstance().format behaviou for the moment.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I have also documented SimpleTemplater and provided a short example at the top of the file.
I will create the Wiki howto and add it to this artifact. Like this if the proposal ever makes it to an OpenXava release, I will just have to copy this Wiki content into the OpenXava Wiki.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Added the Wiki content.
Removed the dependencies to Liferay in SimpleHtmlReportAction: I did the subclass but forgot to remove the code from the main class, sorry.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Reviewing the code one last time, I realised It made a lot more sense to throw exceptions from SimpleTemplater rather than swallow them and return the error in the report or the mail. At least this is the way OpenXava do when there is an error in the code.
I have changed SimpleTemplater and made TemplaterException public.
Changes are included in v2.2 below.
No more changes !!
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
> I realised It made a lot more sense to
> throw exceptions from SimpleTemplater rather than swallow them and return
> the error in the report or the mail
I agree with you. I saw it the first time, but given that it was a private exception I didn't say anything. Of course, I like more in the new way you use.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Although I said, I would not, I made a few changes:
- Removed the SimpleLiferayHtmlReportAction (the functions have been moved as static functions in LiferayDocumentLibrary. Better prepare for the removal of the dependency to Liferay.
- Refactored TemplaterException into SimpleTemplaterException
All included in v2.3
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Laurent,
I have added your code to OpenXava. It is already in the SVN and it will be part of OX4.3.
I done some slight modifications:
1. I added JUnit test: It is very important. Your code contribution is very complete with doc and examples, but having junit test is very important in order to preserver the behavior of your code in future OX versions. These are the junit test classes:
package org.openxava.test.tests;
2. I renamed your sample classes to Coporation and CorporationEmployee because Company and Person already exist in OpenXavaTest.
3. I removed SimpleHTMLMailBaseAction because there are no sample code to test it. I don't want to add not tested code to OX. Anyways we can add this class in the future, but with sample code and junit tests.
4. I moved methods you added to Objects to SimpleHTMLReportAction so instead of writing Objects.getEntityParameters(entity) you have to write getEntityParameters(entity). These method are only used from your action so it's better to put them only in your action. When we need the functionality of the method from other parts of OX we'll move it back to Objects. My idea in general is creating generic code when the functionality is used for third time. When you create generic code from the very start, the code is very difficult to reuse in the future. You cannot create generic code until you have several different use cases.
5. I changed Vector by Collection
6. I put all you error messages in i18n files. All OpenXava messages, even those produces by exceptions, must be localized.
7. I changed SimpleTemplaterException by XavaException, because in any part of the code the developer need to differenciate between a generic runtime exception and a SimpleTemplaterException. When we need it we'll add the specific exception. By now, XavaException is a runtime exception that is throwsn when an unexpected problem is produced.
8. I removed some elements that are out of the scope of this feature: Test (a model class), upload file controller, valueWithUnitEditor.jsp and Liferay utility classes. These classes are not used for testing the simple html report functionality.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
A stripped down version of an application
Hi Laurent,
I think your contribution is very useful, and the code has a good quality. However, I ask your some modification in order to be coherent in style with the already existing OX code.
These are my requests:
1. Rename LiferayLibraryUtil to LiferayDocumentLibrary: The other classes from org.openxava.util do not use the suffix 'Util'
2. Integrate the methods in ObjectUils in the already existing Objects
3. Use the already existing Emails intestad of MailUtils
4- Remove the references to Liferay code in SimpleHtmlReportAction: You utility can be used from outside of Liferay or from other portals. You can move this code to another classes (a subclass maybe?)
5. Use WebEditors.format instead of FormatUtils.
Hi Javier,
1. Done
2. Done
3. Done: I have not looked enough at your code, I should have used this right from the beginning...
4. Done
I have created a new class which extends SimpleHtmlReportAction called SimpleLiferayHtmlReportAction.
The getLiferayLibraryImages allows for very easy production of professional datasheets...
5. I have removed FormatUtils but I did not get the howto for your comment...
I have therefore relied on the DateFormat.getInstance().format behaviou for the moment.
I have also documented SimpleTemplater and provided a short example at the top of the file.
I will create the Wiki howto and add it to this artifact. Like this if the proposal ever makes it to an OpenXava release, I will just have to copy this Wiki content into the OpenXava Wiki.
Added the Wiki content.
Removed the dependencies to Liferay in SimpleHtmlReportAction: I did the subclass but forgot to remove the code from the main class, sorry.
Includes requested changes and the Wiki howto
Good work!
I'll integrate your code in OX
Made SimpleTemplater comply to OpenXava standard
Reviewing the code one last time, I realised It made a lot more sense to throw exceptions from SimpleTemplater rather than swallow them and return the error in the report or the mail. At least this is the way OpenXava do when there is an error in the code.
I have changed SimpleTemplater and made TemplaterException public.
Changes are included in v2.2 below.
No more changes !!
> I realised It made a lot more sense to
> throw exceptions from SimpleTemplater rather than swallow them and return
> the error in the report or the mail
I agree with you. I saw it the first time, but given that it was a private exception I didn't say anything. Of course, I like more in the new way you use.
Although I said, I would not, I made a few changes:
- Removed the SimpleLiferayHtmlReportAction (the functions have been moved as static functions in LiferayDocumentLibrary. Better prepare for the removal of the dependency to Liferay.
- Refactored TemplaterException into SimpleTemplaterException
All included in v2.3
Removed SimpleLiferayHtmlReportAction
Laurent,
I have added your code to OpenXava. It is already in the SVN and it will be part of OX4.3.
I done some slight modifications:
1. I added JUnit test: It is very important. Your code contribution is very complete with doc and examples, but having junit test is very important in order to preserver the behavior of your code in future OX versions. These are the junit test classes:
package org.openxava.test.tests;
import org.openxava.tests.*;
/**
* @author Javier Paniza
*/
class CorporationTest extends ModuleTestBase {
}
package org.openxava.test.tests;
import org.openxava.tests.*;
/**
* @author Javier Paniza
*/
class CorporationEmployeeTest extends ModuleTestBase {
}
2. I renamed your sample classes to Coporation and CorporationEmployee because Company and Person already exist in OpenXavaTest.
3. I removed SimpleHTMLMailBaseAction because there are no sample code to test it. I don't want to add not tested code to OX. Anyways we can add this class in the future, but with sample code and junit tests.
4. I moved methods you added to Objects to SimpleHTMLReportAction so instead of writing Objects.getEntityParameters(entity) you have to write getEntityParameters(entity). These method are only used from your action so it's better to put them only in your action. When we need the functionality of the method from other parts of OX we'll move it back to Objects. My idea in general is creating generic code when the functionality is used for third time. When you create generic code from the very start, the code is very difficult to reuse in the future. You cannot create generic code until you have several different use cases.
5. I changed Vector by Collection
6. I put all you error messages in i18n files. All OpenXava messages, even those produces by exceptions, must be localized.
7. I changed SimpleTemplaterException by XavaException, because in any part of the code the developer need to differenciate between a generic runtime exception and a SimpleTemplaterException. When we need it we'll add the specific exception. By now, XavaException is a runtime exception that is throwsn when an unexpected problem is produced.
8. I removed some elements that are out of the scope of this feature: Test (a model class), upload file controller, valueWithUnitEditor.jsp and Liferay utility classes. These classes are not used for testing the simple html report functionality.