11:35 a.m.
On Mon, Dec 9, 2013 at 4:44 PM, Lucas Ponce <lponce(a)redhat.com> wrote:
Hello Hai,
Sorry for my delay, it was public holiday past friday and I coudn't
comment until now.
Some comments in-line:
----- Mensaje original -----
> De: "Hai Nguyen" <haint(a)exoplatform.com>
> Para: "gatein-dev" <gatein-dev(a)lists.jboss.org>
> Enviados: Jueves, 5 de Diciembre 2013 10:55:14
> Asunto: [gatein-dev] Setup Root password feature improvement
>
> Hi guys,
>
> Currently, the setup root password feature has some disadvantages.
> Those are:
> - This feature depended directly on portal core and could not
enable/disable
> feature
This feature can be disable removing PortalSetupFilter from
portal.war/WEB-INF/web.xml and replacing
SetupOrganizationDatabaseInitializer per OrganizationDatabaseInitializer in
organization-configuration.xml.
But we aim to not modify web.xml because products would not modify
directly
portal.war (thank to extension mechanism).
> - The SetupOrganizationDatabaseInitializer was clone of
> OrganizationDatabaseInitializer (in core service), that is hard for
maintain
Yes, I agree that clone was not more elegant way, but there was not
possibility to extend OrganizationDatabaseInitializer with a custom class,
so clone was the only option to add custom behaviour.
I would suggest to give the ability to extend this class and override
logic from other localization rather that exo.core module.
IMO, the new behaviors are not much to introduce a new custom class.
Beside, I
don't know why we need introduce more "updateUsers" property that
is not necessary. If you looking for my PR, you will see we don't need
SetupOrganizationDatabaseInitializer for this feature
> - The implementation of PortalSetupService is not really good
because it
used
> many static methods and hardcode properties
Every code always can be improved, for sure.
Some comments about static methods:
- There is a static variable that is used to provide a flag about if setup
is complete (isSetup()) method. Methods that works with this static
variable are also static.
- This class also can be used from a portal-setup.sh script, so for that
particular scenario are also used static methods.
About properties, I agree that they can be read it from a
configuration.properties file, but at time we coded it, we didn't see a
clear scenario for change, so that was main reason to not change it, but I
agree that those properties can be also read it from a file.
Take a look my change. Actually, the PortalSetupService needs only static
method that is "encodePassword(String plainTextPassword)" for
PortalSetupCommand. The PortalSetupService should respect normal GateIn
service that means it should be declared and configured in a configuration
xml file.
> - Potential buggy https://issues.jboss.org/browse/GTNPORTAL-3320
>
This issue is a expected behaviour as password should be encoded using
portal-setup.sh/.bat script.
No, this issue relates to JCR session. We could fix it by
RequestLifeCycle.begin/end().
It was fixed in my PR, of course
> We have collected issues of this feature and created
> https://issues.jboss.org/browse/GTNPORTAL-3315 to improve it
>
> Who can review or have idea about that?
>
I don't see bugs related to this module.
I agree that we should write a better note about it, sorry for that.
I can understand that may be we can unify criterion about where to place
filters or if all properties have to be read from files.
Regards,
Lucas
If you have time, could you help me verify my PR doesn't have any impact
for this module?
Thanks a lot,
> Kind regards,
>
> _______________________________________________
> gatein-dev mailing list
> gatein-dev(a)lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/gatein-dev