On Mon, Dec 9, 2013 at 4:44 PM, Lucas Ponce <lponce@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@exoplatform.com>
> Para: "gatein-dev" <gatein-dev@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@lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/gatein-dev