Re: Policy on commit

[Jacques Le Roux]

Please help yourself: http://ofbiz.apache.org/mailing-lists.html

Jacques


Le 30/08/2018 à 22:24, Kev2600 a écrit :

Unsubscribe

On Thu, Aug 30, 2018 at 7:04 AM Jacques Le Roux <
jacques.le.r...@les7arts.com> wrote:


Thanks Nicolas,

I'll wait a bit more for opinions and if we mostly agree will being to
write a draft.

Not a hurry, but of course all opinions, and especially ideas even if in
draft form , are welcome!

Jacques


Le 30/08/2018 à 11:20, Nicolas Malin a écrit :

Hi Jacques,

I agree with the main Michael's idea and yours to load it as best

practice.

Now I'm not the better man to rewrite a formulation. But if no

inspiration here I can try a first prose

Nicolas


On 27/08/2018 10:15, Jacques Le Roux wrote:

Hi Michael, All,

First, thank you Michael for your effort in trying to clarify what to

discuss in dev ML (this has already been , when to revert a commit, and
I'll

add relations with Jiras status.
I know it's important for you to correctly deliver the information

about OFBiz activity in the monthly blog post

My goal here is to decide if we should write best practices for that in


https://cwiki.apache.org/confluence/display/OFBIZ/OFBiz+Contributors+Best+Practices

<

https://cwiki.apache.org/confluence/display/OFBIZ/OFBiz+Contributors+Best+Practices

And if yes, to clearly write them. This to avoid future confusion and

conflicts in the community about these subjects.

Before beginning on that, I have already collected some information,

I'd like to be sure we all agree about doing so. Then, if we agree we can

begin to discuss what to write...

Thanks for your attention

Jacques


Le 19/08/2018 à 11:28, Michael Brohl a écrit :

I have not the time to dig into the specific details right now so will

just give my thoughts on the process in general because of the citations:

1. we have to distinguish between (a) completely new functionality or

major refactorings and (b) the enhancement of functionality which is
already

in the code base

2. for (a), we should first have consenus that we want the proposed

solution and we should look for a complete, well designed and carefully
tested

solution before the first commit will be done. This is to prevent the

introduction of new problematic code.

3. for (b), I think every improvement of existing code/functionality

helps and should be committed if there are no flaws in the design or

technical solution and it does not break existing funtionality. of

course, it should be complete within the *scope* of the improvement.

4. if the solution for (b) does not cover other wishes or things which

could be enhanced also, this would be no reason to not commit the

improvement. If people have further requirements, they can provide

concepts and solutions/patches anytime to make things better.

In this case, for me it is important if Suraj's commit

a. breaks anything?

b. is vetoed by other committers in view of code quality or design

flaws?

If none of these questions get a "yes", then I see no reason to revert.

If you have additional requirements, you are encouraged to provide

solutions or concepts for them.

Thanks,

Michael Brohl
ecomify GmbH
www.ecomify.de

Re: Policy on commit

[Kev2600]

Unsubscribe

On Thu, Aug 30, 2018 at 7:04 AM Jacques Le Roux <
jacques.le.r...@les7arts.com> wrote:

> Thanks Nicolas,
>
> I'll wait a bit more for opinions and if we mostly agree will being to
> write a draft.
>
> Not a hurry, but of course all opinions, and especially ideas even if in
> draft form , are welcome!
>
> Jacques
>
>
> Le 30/08/2018 à 11:20, Nicolas Malin a écrit :
> > Hi Jacques,
> >
> > I agree with the main Michael's idea and yours to load it as best
> practice.
> >
> > Now I'm not the better man to rewrite a formulation. But if no
> inspiration here I can try a first prose
> >
> > Nicolas
> >
> >
> > On 27/08/2018 10:15, Jacques Le Roux wrote:
> >> Hi Michael, All,
> >>
> >> First, thank you Michael for your effort in trying to clarify what to
> discuss in dev ML (this has already been , when to revert a commit, and
> I'll
> >> add relations with Jiras status.
> >> I know it's important for you to correctly deliver the information
> about OFBiz activity in the monthly blog post
> >>
> >> My goal here is to decide if we should write best practices for that in
> >>
> https://cwiki.apache.org/confluence/display/OFBIZ/OFBiz+Contributors+Best+Practices
> >> <
> https://cwiki.apache.org/confluence/display/OFBIZ/OFBiz+Contributors+Best+Practices
> >
> >>
> >> And if yes, to clearly write them. This to avoid future confusion and
> conflicts in the community about these subjects.
> >>
> >> Before beginning on that, I have already collected some information,
> I'd like to be sure we all agree about doing so. Then, if we agree we can
> >> begin to discuss what to write...
> >>
> >> Thanks for your attention
> >>
> >> Jacques
> >>
> >>
> >> Le 19/08/2018 à 11:28, Michael Brohl a écrit :
> >>> I have not the time to dig into the specific details right now so will
> just give my thoughts on the process in general because of the citations:
> >>>
> >>> 1. we have to distinguish between (a) completely new functionality or
> major refactorings and (b) the enhancement of functionality which is
> already
> >>> in the code base
> >>>
> >>> 2. for (a), we should first have consenus that we want the proposed
> solution and we should look for a complete, well designed and carefully
> tested
> >>> solution before the first commit will be done. This is to prevent the
> introduction of new problematic code.
> >>>
> >>> 3. for (b), I think every improvement of existing code/functionality
> helps and should be committed if there are no flaws in the design or
> >>> technical solution and it does not break existing funtionality. of
> course, it should be complete within the *scope* of the improvement.
> >>>
> >>> 4. if the solution for (b) does not cover other wishes or things which
> could be enhanced also, this would be no reason to not commit the
> >>> improvement. If people have further requirements, they can provide
> concepts and solutions/patches anytime to make things better.
> >>>
> >>> In this case, for me it is important if Suraj's commit
> >>>
> >>> a. breaks anything?
> >>>
> >>> b. is vetoed by other committers in view of code quality or design
> flaws?
> >>>
> >>> If none of these questions get a "yes", then I see no reason to revert.
> >>>
> >>> If you have additional requirements, you are encouraged to provide
> solutions or concepts for them.
> >>>
> >>> Thanks,
> >>>
> >>> Michael Brohl
> >>> ecomify GmbH
> >>> www.ecomify.de
> >>
> >>
> >
> >
>
>

Re: OFBIZ-10307: Navigate from a domain to another with automated signed in authentication

[Jacques Le Roux]

Hi Nicolas,

Thanks for your feedback, much appreciated. I know it's not something easy to 
review and even less to test.

About the names, I see no problems to change them. I used them to try to clarify what they do. Actually for the js functions I initially used names 
close to what you suggest. And for checkExternalServerLogin I was inspired by checkExternalLoginKey.


I'll be happy to change them if everybody agree about that. They are short and 
explicit enough in the context indeed.

Note though that Deepak created the JWTManager class in order to use JWTs for 
more than an use with userLogin.

Jacques


Le 30/08/2018 à 13:58, Nicolas Malin a écrit :
Globally (after Taher remarks :) ) I found the code clear. Instead of my comment about the jwt generation [1] on issue OFBIZ-9833 [2] I haven't 
other technical remarks that can help to improve this.


Now on naming vision I suggest simple naming like this :

setUserLoginIdInJwtToken -> loadJWT()
sendUserLoginIdInJwtToken -> sendJWT()
checkExternalServerLogin -> checkJWTLogin()

Because I didn't see why we call a token without userLogin and jwt always talk 
about a token (Jsin Web Token) not necessary to redound it

Nicolas

[1] 
https://issues.apache.org/jira/browse/OFBIZ-9833?focusedCommentId=16594702=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16594702

[2] https://issues.apache.org/jira/browse/OFBIZ-9833
On 29/08/2018 12:37, Jacques Le Roux wrote:

I'll soon provide an AsciiDoc for that. I have to think about its location, any 
ideas?

Jacques


Le 29/08/2018 à 12:06, Taher Alkhateeb a écrit :

I couldn't find the testing mechanism and the JIRA is filled with too
much information. Sharing a simple testing mechanism would help
On Wed, Aug 29, 2018 at 12:33 PM Jacques Le Roux
 wrote:

Hi Taher,

Before creating the last patch, I have reviewed the code a last time again,  
it's totally OK now. Did you test from
https://localhost:8443/catalog/control/FindCatalog with the provided test patch?

I'll now provide a simple and concise AsciiDoc documentation. Maybe with a 
graph, but that's not sure, I have to try...

For the test part I'll wait for the user ML "OFBiz Sanity Test Automation" 
thread https://markmail.org/message/nxoyqxn7lqfupf55 to conclude before
endeavouring in web tests

I don't think it should stop to commit this work, which can be tested manually 
using the provided patches (though the one from example needs the
commit to be done)

Anyway I'll wait more feedbacks before committing, I'm not in a hurry and I 
know it's a sensitive feature.

Jacques


Le 27/08/2018 à 14:55, Taher Alkhateeb a écrit :

I already found problems in this code to give me pause and to show
that it is not properly studied and designed. If you want to continue
the effort, then I recommend reviewing your code thoroughly, providing
a clear testing mechanism (especially the web scripts), and ask for
reviews and tests.
On Sun, Aug 26, 2018 at 12:33 PM Jacques Le Roux
 wrote:

Hi Taher,

Thanks for your review, much appreciated.


Le 22/08/2018 à 18:42, Taher Alkhateeb a écrit :

I reviewed the code and explanation in the beginning of this thread
and I have the following comments:

- First, in case of a result.containsKey(ModelService.ERROR_MESSAGE)
you are still returning "success". Does not make sense at all.

You are right I should have used "error".
I did not notice (actually forgot a last own review after much variations in 
code) because it's an event preprocessor and no response is
expected/handled.
Not to bad, in case of error just an EventHandlerException was not thrown.


- Also the comments are unnecessary and do not make sense in the same code block

You mean "// Something unexpected happened here", or ?


- The comment you wrote: "Check it's the right tenant in case username
and password are the same in different tenants. Not sure this is
really useful in the case of external server, does not hurt anyway" is
self-defeating. Never put in code you won't use. We have enough mess
in the code base.

I must say I initially copied that from 
ExternalLoginKeysManager::checkExternalLoginKey and adapted it to the context. 
It's roughly the same code.
I let it there because I was then unsure. But when you think about it, there is no reason to not make this check. So I'll simply remove the 
comment. I

should have put a TODO.

BTW this is out of subject and I'll start a new thread about it after reading
https://www.linkedin.com/pulse/architecture-constraints-end-multi-tenancy-gregor-hohpe/
The subject will be: "Should we keep the multi-tenants feature in OFBiz."
I though think the tenants in OFBiz are still useful when you only needs dozens 
or maybe even few hundreds tenants (begin to be a lot of DBs!).
But I faced that with a startup which wanted to handle thousands, if not millions (actually they failed), of tenants, obviously OFBiz can't do 
that.



- Calling LoginWorker.doBasicLogin(userLogin, request) after the
if/else block does not 

Re: [PROPOSITION] Demos: replace old by trunk framework only

[Nicolas Malin]

I forgot to reply :), after sharing with my coworkers, It's would b 
interesting for us to works on this parts.


So I will start a POC just to see if we go on a good way.

Nicolas


On 25/08/2018 13:35, Nicolas Malin wrote:

On 24/08/2018 09:16, Taher Alkhateeb wrote:
Furthermore, I'm not sure we need another superman initiative in the 
widget

system? The price to pay for this does not seem to be worth the added
value, I would hardly call it a value but maybe I'm wrong?
I'm the feeling that it's not a mountain (less that common-theme :) ). 
I will sharing in face to face tomorrow if we have at Néréide a 
benefit for an improvement like that and if we are ready consume some 
time to create a POC to trail it.


Nicolas

Re: OFBIZ-10307: Navigate from a domain to another with automated signed in authentication

[Nicolas Malin]

Globally (after Taher remarks :) ) I found the code clear. Instead of my 
comment about the jwt generation [1] on issue OFBIZ-9833 [2] I haven't 
other technical remarks that can help to improve this.


Now on naming vision I suggest simple naming like this :

setUserLoginIdInJwtToken -> loadJWT()
sendUserLoginIdInJwtToken -> sendJWT()
checkExternalServerLogin -> checkJWTLogin()

Because I didn't see why we call a token without userLogin and jwt 
always talk about a token (Jsin Web Token) not necessary to redound it


Nicolas

[1] 
https://issues.apache.org/jira/browse/OFBIZ-9833?focusedCommentId=16594702=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16594702

[2] https://issues.apache.org/jira/browse/OFBIZ-9833
On 29/08/2018 12:37, Jacques Le Roux wrote:
I'll soon provide an AsciiDoc for that. I have to think about its 
location, any ideas?


Jacques


Le 29/08/2018 à 12:06, Taher Alkhateeb a écrit :

I couldn't find the testing mechanism and the JIRA is filled with too
much information. Sharing a simple testing mechanism would help
On Wed, Aug 29, 2018 at 12:33 PM Jacques Le Roux
 wrote:

Hi Taher,

Before creating the last patch, I have reviewed the code a last time 
again,  it's totally OK now. Did you test from
https://localhost:8443/catalog/control/FindCatalog with the provided 
test patch?


I'll now provide a simple and concise AsciiDoc documentation. Maybe 
with a graph, but that's not sure, I have to try...


For the test part I'll wait for the user ML "OFBiz Sanity Test 
Automation" thread https://markmail.org/message/nxoyqxn7lqfupf55 to 
conclude before

endeavouring in web tests

I don't think it should stop to commit this work, which can be 
tested manually using the provided patches (though the one from 
example needs the

commit to be done)

Anyway I'll wait more feedbacks before committing, I'm not in a 
hurry and I know it's a sensitive feature.


Jacques


Le 27/08/2018 à 14:55, Taher Alkhateeb a écrit :

I already found problems in this code to give me pause and to show
that it is not properly studied and designed. If you want to continue
the effort, then I recommend reviewing your code thoroughly, providing
a clear testing mechanism (especially the web scripts), and ask for
reviews and tests.
On Sun, Aug 26, 2018 at 12:33 PM Jacques Le Roux
 wrote:

Hi Taher,

Thanks for your review, much appreciated.


Le 22/08/2018 à 18:42, Taher Alkhateeb a écrit :

I reviewed the code and explanation in the beginning of this thread
and I have the following comments:

- First, in case of a result.containsKey(ModelService.ERROR_MESSAGE)
you are still returning "success". Does not make sense at all.

You are right I should have used "error".
I did not notice (actually forgot a last own review after much 
variations in code) because it's an event preprocessor and no 
response is

expected/handled.
Not to bad, in case of error just an EventHandlerException was not 
thrown.


- Also the comments are unnecessary and do not make sense in the 
same code block

You mean "// Something unexpected happened here", or ?

- The comment you wrote: "Check it's the right tenant in case 
username

and password are the same in different tenants. Not sure this is
really useful in the case of external server, does not hurt 
anyway" is

self-defeating. Never put in code you won't use. We have enough mess
in the code base.
I must say I initially copied that from 
ExternalLoginKeysManager::checkExternalLoginKey and adapted it to 
the context. It's roughly the same code.
I let it there because I was then unsure. But when you think about 
it, there is no reason to not make this check. So I'll simply 
remove the comment. I

should have put a TODO.

BTW this is out of subject and I'll start a new thread about it 
after reading
https://www.linkedin.com/pulse/architecture-constraints-end-multi-tenancy-gregor-hohpe/ 

The subject will be: "Should we keep the multi-tenants feature in 
OFBiz."
I though think the tenants in OFBiz are still useful when you only 
needs dozens or maybe even few hundreds tenants (begin to be a lot 
of DBs!).
But I faced that with a startup which wanted to handle thousands, 
if not millions (actually they failed), of tenants, obviously 
OFBiz can't do that.



- Calling LoginWorker.doBasicLogin(userLogin, request) after the
if/else block does not make sense. The else block guarantees falling
into the exception. This whole code block needs refactoring
You are totally right, I have uploaded a new patch where I put the 
call of

   LoginWorker.doBasicLogin(userLogin, request);
in the positive part of the if and used the same block pattern 
than above where there is a warning log then returning an error.
I was so focused on the other parts of the mechanism, especially 
to secure it, than I forgot to review the Java part.

My bad, thank you again for your review.

- A testing method needs to be provided in detail. For example 
I'm not

sure how the Javascript portions will 

Re: Policy on commit

[Jacques Le Roux]

Thanks Nicolas,

I'll wait a bit more for opinions and if we mostly agree will being to write a 
draft.

Not a hurry, but of course all opinions, and especially ideas even if in draft 
form , are welcome!

Jacques


Le 30/08/2018 à 11:20, Nicolas Malin a écrit :

Hi Jacques,

I agree with the main Michael's idea and yours to load it as best practice.

Now I'm not the better man to rewrite a formulation. But if no inspiration here 
I can try a first prose

Nicolas


On 27/08/2018 10:15, Jacques Le Roux wrote:

Hi Michael, All,

First, thank you Michael for your effort in trying to clarify what to discuss in dev ML (this has already been , when to revert a commit, and I'll 
add relations with Jiras status.

I know it's important for you to correctly deliver the information about OFBiz 
activity in the monthly blog post

My goal here is to decide if we should write best practices for that in 
https://cwiki.apache.org/confluence/display/OFBIZ/OFBiz+Contributors+Best+Practices



And if yes, to clearly write them. This to avoid future confusion and conflicts 
in the community about these subjects.

Before beginning on that, I have already collected some information, I'd like to be sure we all agree about doing so. Then, if we agree we can 
begin to discuss what to write...


Thanks for your attention

Jacques


Le 19/08/2018 à 11:28, Michael Brohl a écrit :

I have not the time to dig into the specific details right now so will just 
give my thoughts on the process in general because of the citations:

1. we have to distinguish between (a) completely new functionality or major refactorings and (b) the enhancement of functionality which is already 
in the code base


2. for (a), we should first have consenus that we want the proposed solution and we should look for a complete, well designed and carefully tested 
solution before the first commit will be done. This is to prevent the introduction of new problematic code.


3. for (b), I think every improvement of existing code/functionality helps and should be committed if there are no flaws in the design or 
technical solution and it does not break existing funtionality. of course, it should be complete within the *scope* of the improvement.


4. if the solution for (b) does not cover other wishes or things which could be enhanced also, this would be no reason to not commit the 
improvement. If people have further requirements, they can provide concepts and solutions/patches anytime to make things better.


In this case, for me it is important if Suraj's commit

a. breaks anything?

b. is vetoed by other committers in view of code quality or design flaws?

If none of these questions get a "yes", then I see no reason to revert.

If you have additional requirements, you are encouraged to provide solutions or 
concepts for them.

Thanks,

Michael Brohl
ecomify GmbH
www.ecomify.de

Re: Policy on commit

[Nicolas Malin]

Hi Jacques,

I agree with the main Michael's idea and yours to load it as best practice.

Now I'm not the better man to rewrite a formulation. But if no 
inspiration here I can try a first prose


Nicolas


On 27/08/2018 10:15, Jacques Le Roux wrote:

Hi Michael, All,

First, thank you Michael for your effort in trying to clarify what to 
discuss in dev ML (this has already been , when to revert a commit, 
and I'll add relations with Jiras status.
I know it's important for you to correctly deliver the information 
about OFBiz activity in the monthly blog post


My goal here is to decide if we should write best practices for that 
in 
https://cwiki.apache.org/confluence/display/OFBIZ/OFBiz+Contributors+Best+Practices
 



And if yes, to clearly write them. This to avoid future confusion and 
conflicts in the community about these subjects.


Before beginning on that, I have already collected some information, 
I'd like to be sure we all agree about doing so. Then, if we agree we 
can begin to discuss what to write...


Thanks for your attention

Jacques


Le 19/08/2018 à 11:28, Michael Brohl a écrit :
I have not the time to dig into the specific details right now so 
will just give my thoughts on the process in general because of the 
citations:


1. we have to distinguish between (a) completely new functionality or 
major refactorings and (b) the enhancement of functionality which is 
already in the code base


2. for (a), we should first have consenus that we want the proposed 
solution and we should look for a complete, well designed and 
carefully tested solution before the first commit will be done. This 
is to prevent the introduction of new problematic code.


3. for (b), I think every improvement of existing code/functionality 
helps and should be committed if there are no flaws in the design or 
technical solution and it does not break existing funtionality. of 
course, it should be complete within the *scope* of the improvement.


4. if the solution for (b) does not cover other wishes or things 
which could be enhanced also, this would be no reason to not commit 
the improvement. If people have further requirements, they can 
provide concepts and solutions/patches anytime to make things better.


In this case, for me it is important if Suraj's commit

a. breaks anything?

b. is vetoed by other committers in view of code quality or design 
flaws?


If none of these questions get a "yes", then I see no reason to revert.

If you have additional requirements, you are encouraged to provide 
solutions or concepts for them.


Thanks,

Michael Brohl
ecomify GmbH
www.ecomify.de