Hello Hai,
I've verified your PR.
I don't see any potential impact on it, so I'm ok with the change.
For some reason, we coded outside kernel but including as a startable component is a
change that makes sense.
Thanks,
Lucas
----- Mensaje original -----
De: "Hai Nguyen" <haint(a)exoplatform.com>
Para: "Lucas Ponce" <lponce(a)redhat.com>
CC: "gatein-dev" <gatein-dev(a)lists.jboss.org>
Enviados: Lunes, 9 de Diciembre 2013 11:35:26
Asunto: Re: [gatein-dev] Setup Root password feature improvement
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
>