IDempiere/FullMeeting20130206

⇐ Table of Contents | Full Meeting Minutes | Full Meeting 2013-02-06

CarlosRuiz: Good Morning
nmicoud_: Bonjour
tbayen: Daarestiet. :-)
JanThielemann: hello carlos
JanThielemann: i made a jira ticket like you said, may you take a look at it? http://jira.idempiere.com/browse/IDEMPIERE-612
tbayen: I would like to talk about IDEMPIERE-90. It blocks the ongoing contributions for the Swing Branch.
tbayen: Does anything speak against nmicoud_ 's contribution for IDEMPIERE-90? For me it works well, but I did not test with zk.
CarlosRuiz: looks good Jan - I think nmicoud_ comment make sense - most of icons actually are png
nmicoud_: zk works well (for me at least), but could be enhanced (at the moment, you have to make a right click to change mode ; it would be better to have a padlock button)
Deepak: Good Morning Everybody
CarlosRuiz: Hi Deepak
Deepak: Carlos, I committed a fix to web service
Deepak: That was related to ctx was wiped out due to call of Env.setCtx
Deepak: I informed Richard about same fix
CarlosRuiz: yes - Richard told me it fixed and now all tests passed
CarlosRuiz: thanks for that
Deepak: I do not know who is worked on change role task
Deepak: But Env.setCtx is used on that too
Deepak: We should replace it with ServerContext.setCurrentInstance to avoid possible issue of ctx wipe out
tbayen: JanThielemann, I would also agree with nmicoud_ . If one wants to do a redesign package he will change all kinds of images and icons. For me it makes sense to hold them in one table.
CarlosRuiz: Deepak - Env.setCtx is still on change role?
Deepak: Yes
Deepak: I looked for all reference and I found one instance which is on change role implementation
CarlosRuiz: do you have the line number? I saw hengsin fixed most of those recently
Deepak: Let me check
Deepak: In LoginWindow.changeRole method, first line is doing that
Deepak: I did not changed it as change role is working perfectly and after changing this we may need to test it so that it do not break existing functionality
hengsin: deepak. that one should be ok. it is means to wipe out the original context
Deepak: ohk
hengsin: IDEMPIERE-90 - is that based on actual usage or is a list that someone have to set it up ?
tbayen: hengsin, it is set up by a field in the table. If a field with this name exists the functionality is switched on.
nmicoud_: and the default value for the column is Y, so current record are immedialtely in the short list
tbayen: I would not add this to any of the standard tables but let the user add this column if he wants to.
nmicoud_: exactly ; that could be used like the QuickEntry field. It is there, and choose to use it or not
hengsin: I think "20 most used" should derived from actual usage instead of artificially set up.
buildmaster: Project iDempiere build #736: SUCCESS in 8 min 5 sec: http://jenkins.idempiere.com/job/iDempiere/736/
buildmaster: globalqss: IDEMPIERE-600 Use User Org Access in Role didn't work - thanks to Juliana Corredor
hengsin: Jan, for IDEMPIERE-612, web app's icon is usually png/gif instead of ico
JanThielemann: i know
JanThielemann: the thing is, zk want a url to the image
CarlosRuiz: hengsin, do you mean "update table set isshortlist='Y'" must be set based on usage stats? or that the program must check for them?
JanThielemann: but there is no url if you try to load icons from the database
JanThielemann: but setting the imagecontent is possible and works well
hengsin: yeah, so there's one difference there between swing and zk's requirement that needs to be clarify. another issue of having icon in db is it is bad for performance especially for web application ( db hit and browser cache issue )
JanThielemann: the icons get cached. i don't think that performance is so bad
hengsin: @Carlos, I guess there should be a table to collect that usage statistics by user or client
JanThielemann: maybe you want to review my code and improve it when i am done
nmicoud_: for idempiere-90, it could be possible to add this through a process, to update the flag. But, for some table, (eg greetings), you only want to see 5 items (whereas there are maybe 30). And this can only be done manually
hengsin: jan, would setimagecontent works with browser cache ?
JanThielemann: hengsin i think so
JanThielemann: the icon gets cached on the server side so the database is only queried once
JanThielemann: the rest is like before i think
tbayen: hengsin, nmicoud_ I think we need both: A settable column for things like greetings and a usage statistic for the other. And the number of items has to be configurable.
tbayen: This are two different issues.
nmicoud_: or, maybe the 'statistic' is an evolution ?
CarlosRuiz: tbayen, about your pull request 62
tbayen: yes
CarlosRuiz: you're right that was broken by IDEMPIERE-480 - thanks for the finding
CarlosRuiz: C_BankStatement.DateAcct didn't exist before
CarlosRuiz: now - your patch just fixes the import - but the fix must be more generic
tbayen: I tried to do it with as low impact as possible. For me the "right" solution would be your first solution: Make sure that lines are in the same period than the statement. I am a bit unhappy with your solution but I did not have the time.
tbayen: I would like to have a bank statement with lines which have different accounting dates.
CarlosRuiz: that's not possible after IDEMPIERE-480
tbayen: Something like a "weekly statement".
tbayen: No, it is no more possible. I can live with that. But I think it is not right.
nmicoud_: I have also this requirement, but for a monthly statement.
tbayen: So in real we talk about IDEMPIERE-480. I feared that.... ;-)
CarlosRuiz: well - the multi-period document was breaking a basic accounting principle
nmicoud_: maybe this can be achieved through a AD_SysConfig variable ? allow different dates on bank stmt, and posting based on thoses date
nmicoud_: dates have to be on the same period
nmicoud_: you don't break any accounting principle
CarlosRuiz: is very hard to control that - you have multi-periods - you can also change the period dates at any time
tbayen: CarlosRuiz, you are right. But we could check for the same period. Why not?!? I could not see your motivation from your commit. It may be that you choosed the easy solution just because the adaxa patch was already there.
CarlosRuiz: no tbayen - the motivation is to avoid any source of data corruption
tbayen: For what reason will you change period dates???
CarlosRuiz: ask users :-D
tbayen: If you change period dates this is a way straight to hell not only for our issue but for every kind of document. Whoever does that has to know what he does. It is the same like "reopening" a period. Every accountant knows that this is forbidden and that you have to be very cautious.
CarlosRuiz: the only document posing a risk was bank statement - now is not a risk
tbayen: Is it because bank statement is the only document that has (possibly) more than one accounting date?
CarlosRuiz: more than one accounting date sounds fine - more than one accounting period sounds data corruption - so I vote for safety
tbayen: hengsin, do you have a caveat if we accept IDEMPIERE-90 like it is now and open another issue for the usage statistics shortlist?
nmicoud_: for bank statement, what about adding c_period_id on header and let user choose line date within the period ? and use the 'old' posting way ?
tbayen: CarlosRuiz, can we guarantee that all acounting dates are in one period? And can we hold this guarantee if the user does funny things?
nmicoud_: that would be ok for everybody ?
tbayen: nmicoud_, If I understand CarlosRuiz right, Users change period dates...
tbayen: Hi red1
nmicoud_: that's bad. "my" users never did this (for now...)
tbayen: IDEMPIERE-480 came through "Reset Accounting". I do not know how this works. Does it work everything by sql commands or does it ask the Doc_* class what to do?
CarlosRuiz: tbayen, nmicoud_, there is also ability to configure multi-calendars - and multi-accounting with different calendars
red1: Hola all, and tbayen
nmicoud_: that's right , c_period should not be used then. But for the majority of installation, there is a single calendar
CarlosRuiz: I still don't get the problem about single acct date - I think that's a basic principle
nmicoud_: So, maybe add a control (based on a basic 12 months calendar) that will ensure that the line date in the same month as the header date could be enough ? (and if set in AD_SysConfog)
CarlosRuiz: you must not open doors in any document to have the possibility (even remote) of spreading in two periods
nmicoud_: For instance, we have a bankstatement (in adempiere) for a bank statement send by the bank ; and it covers 15 days
CarlosRuiz: I guess all of us have faced the issue of fixing data corruption because a user was too creative and adempiere is too relaxed
nmicoud_: :))
CarlosRuiz: it covers 15 days- but the document just have one date - is the doc date
CarlosRuiz: same as an order . it can have several shipment dates - but the document have just one date
nmicoud_: but posting, which will cover several days, need to have the correct dates
tbayen: In real our bank statement are 14 documents. That is what the importer does: It creates 14 documents.
nmicoud_: Users will create a bank statement per day
nmicoud_: and pointing it with 'real' bank statement will be hard
CarlosRuiz: aha - tbayen - importbankstatement have that comment -> // Create a new Bank Statement for every statement date
tbayen: OK - if we see it this way: What is the problem with my patch? Are there problems somewhere else.
CarlosRuiz: no problems with your patch - it fixes the issue for importers
CarlosRuiz: it doesn't fix the issues with any other development using "new MBankStatement"
buildmaster: Project iDempiere build #737: SUCCESS in 8 min 2 sec: http://jenkins.idempiere.com/job/iDempiere/737/
buildmaster: nmicoud: Error with semi-colons
nmicoud_: Can we come back to idempiere-90 ? what do we do about it ?
tbayen: I searched for constructor calls in Eclipse. Only the one with a ResultSet parameter is found in 8 places. This constructor initializes the statement date with now(). We could add a line to initialize dateAcct too.
red1: hengsin: is the Felix 'dashboard' on the demo? Link?
a42niem: hi all
CarlosRuiz: yes - tbayen - that's what I suggested at http://jira.idempiere.com/browse/IDEMPIERE-610
red1: Halo Dirk
CarlosRuiz: sorry - didn't notify you that I added a comment there
tbayen: nmicoud_, for IDEMPIERE-90 I would like to take the patch as it is. For me this functionality is helpful exactly like it is. But we should open a new issue for the usage statistics shortlist.
CarlosRuiz: red1 http://demo.idempiere.com/osgi/system/console
nmicoud_: WDYT Carlos ?
CarlosRuiz: is it working for zk?
red1: Thanks Carlos, what is the login/pass?
tbayen: CarlosRuiz, thanks that you let find me the same solution myself. This gives a good feeling. ;)
nmicoud_: yes
CarlosRuiz: red1, same credentials as adempieremonitor
nmicoud_: but could be enhanced .Actually, it is availabe through right click, no padlock image
red1: SuperUser/System can't work
nmicoud_: use superuser @ idempiere.com / System :)
tbayen: nmicoud_, if it does not do anything without this right click is IMO acceptable. This is the same behaviour before. Or does it hide things and confuse users?
red1: thanks nmicoud_
nmicoud_: no, if there is no isShortList column, you won't see any difference
nmicoud_: if there is such a column, the right click menu will have a new item (short list/full list) ; if you click on it, it switches the content of the fields (as the padlock does)
tbayen: CarlosRuiz, hengsin, what do you think?
red1: Guys... see this http://demo.idempiere.com/osgi/system/console/bundles
red1: type in POS in filter
red1: i just installed POS plugin from my source to the demo
red1: isn't this out of this world?
red1: I click on the Install/Update button and choose my own plugins
red1: kudos to the team :D
red1: http://demo.idempiere.com/webui/
red1: you can see the POS Integration module added !
red1: Now i am going to add LCO WHT, Asset Maintenance and HR Payroll onto it :>
hengsin: alright, we can go ahead with IDEMPIERE-90 and evolve the statistics part later.
nmicoud_: ok, i put the ticket in the review queue
CarlosRuiz: anyways - I think we're basically ready to start a freeze on new functionalities (or very close to that)
CarlosRuiz: so - after integrating IDEMPIERE-90 - for swing can you please focus just on making it work first (bug fixes and implementing zk features) - and not adding new things
nmicoud_: ok, but maybe adding a message on the main jira page ?
CarlosRuiz: yes - agree - we can declare that we're entering in a freeze - so new functionalities will be very carefully chosen (or postponed probably)
nmicoud_: maybe current ticket could be reviewed to say if they have to be resolved during freeze ?
CarlosRuiz: do you mean IDEMPIERE-90?
CarlosRuiz: I'll integrate it - there are a couple of things to fix there
tbayen: CarlosRuiz, I wrote a comment to IDEMPIERE-610. If you agree with what I say I will commit it this evening.
CarlosRuiz: like avoid using the SQL to check the existence of the column
nmicoud_: i was talking about the ticket of thre review queue and others ticket which are still unresolved
nmicoud_: and unassigned
nmicoud_: for instance, this one : http://jira.idempiere.com/browse/IDEMPIERE-609 (a nice bug :))
CarlosRuiz: bug fixes are 100% welcome during freeze period
CarlosRuiz: that's the intention of freeze - to solve bugs mostly
CarlosRuiz: tbayen, disagree if the import behaves different than expected on -480 - both must be consistent
nmicoud_: fine, i'm not sure i can provide fixes, but i could find bugs ;)
tbayen: With only your patch there is no chance to set any date for the statement. MY solution is the nearest to the expected I think.
CarlosRuiz: ah I see
CarlosRuiz: in such case it sounds better to add like this
CarlosRuiz: if (imp.getDateAcct() != null)
CarlosRuiz: {
CarlosRuiz: statement.setDateAcct(imp.getDateAcct());
CarlosRuiz: }
tbayen: Yes, no problem
CarlosRuiz: more flexible
tbayen: The old code (that set the line date) used not dateAcct() but statementLineDate(). I do not understand the difference between them.
CarlosRuiz: :-D
nmicoud_: it's like the DateInvoiced and the DateAcct on C_Invoice
CarlosRuiz: kind of surprising - anyways - that linedate is overwritten now on beforeSave so I'll better comment it out
nmicoud_: document date can be different from acct (but never see a difference between them)
CarlosRuiz: tbayen
tbayen: So you feel like me that getDateAcct() is the right to use. I am confused from the old code "line.setDateAcct(imp.getStatementLineDate());"
CarlosRuiz: ready to commit now - can you please test it after my commit? to be sure everything keeps working?
tbayen: yes, I will do it this evening.
nmicoud_: yes
CarlosRuiz: ok, committed
CarlosRuiz: so, must I take the merge for IDEMPIERE-90 from swing repo?
tbayen: If we do not agree using it I will take it out of idempiere-swing. I like the functionylity. But if you feel it is not ready...?
nmicoud_: the dev is not on swing repo but in my own repo
nmicoud_: we wait for its integration as it make changes on org.adempiere.base
tbayen: nmicoud_, I commited it to swing repo after our discussion about the shortcut was over. But I have to admit that I did not review all the code outside of swing directory.
nmicoud_: ah ok
CarlosRuiz: so - swing repo has the complete code?
tbayen: CarlosRuiz, for me the version in swing repo works and is the newest one. There is a pull request that includes it together with some minor swing issues.
tbayen: pull request #60
CarlosRuiz: is showing a message stating -> 2 new outgoing changes since 14ce647
tbayen: I solved IDEMPIERE-587 since then. You could take it or leave it until further testing. I did not want to discuss it here before talking about the IDEMPIERE-90 code outside of "our" swing directory.
tbayen: If you want now I would talk about IDEMPIERE-587. It does not look well without sensible configuration of all the buttons. You said you wanted to do that. Do you have a migration for that?
CarlosRuiz: it was already committed
buildmaster: Project iDempiere build #738: SUCCESS in 7 min 26 sec: http://jenkins.idempiere.com/job/iDempiere/738/
buildmaster: * Carlos Ruiz <carg67@gmail.com>: Merged in tbayen/idempiere (pull request #59)
buildmaster: IDEMPIERE-596 Report hangs if column for "next line" does not exist
buildmaster: * tbayen: IDEMPIERE-596 Report hangs if column for "next line" does not exist
buildmaster: * globalqss: IDEMPIERE-610 Import of BankStatement does not work (can not create new bank statements) - thanks to Thomas Bayen
tbayen: The state of this contribution is: The code works well and stable but without right configuration it may confuse users. I would like to change the field configurations but did not want to to it at the same time as you work on that.
CarlosRuiz: hmmm - let me check - maybe I didn't commit
tbayen: Perhaps I am some days behind...
CarlosRuiz: ah yes
CarlosRuiz: it's 201301251443_IDEMPIERE-234.sql
tbayen: Hmmm... does not work in my installation. Must be a problem with my personal workflow. Sorry
tbayen: If this is done you can take the whole idempiere-swing now.
CarlosRuiz: already did :-)
CarlosRuiz: now - I'm fixing VLookup to use model instead of direct SQL
tbayen: How long do you think will the freeze last? Does it make sense to know what to do with new contributions in this time? Shall we still create Jira tickets for them?
CarlosRuiz: yes - I think is better to keep new things coming in if they're valuable
CarlosRuiz: the freeze probably must last at least two months - we're not still officially on freeze - but just letting you all know that we're very close to that
CarlosRuiz: so we'll become picky about new things
CarlosRuiz: but I think if we want to keep it rolling we can open a new-features repo - to receive things there and keep in sync with the main repo for the bug fixes
tbayen: We have to see what users needs are. It is a good idea to try to concentrate to solve some old bugs first.
buildmaster: Project iDempiere build #739: SUCCESS in 8 min 29 sec: http://jenkins.idempiere.com/job/iDempiere/739/
buildmaster: * Carlos Ruiz <carg67@gmail.com>: Merged tbayen/idempiere-swing into development
buildmaster: * tbayen: merge with 81f5fd4a0e55
buildmaster: * tbayen: IDEMPIERE-587 - Swing: Toolbar Button to start Process from button fields
tbayen: If I need one or the other new feature for myself I can live with it in my personal branch for two months or so.
buildmaster: * tbayen: IDEMPIERE-587 Swing: Toolbar Button to start Process from button fields
buildmaster: * Nicolas Micoud <nmicoud@tgi.fr>: Merged idempiere/idempiere into development
buildmaster: * tbayen: merge 34174c929304
buildmaster: * tbayen: IDEMPIERE-90 shortcut Ctrl-L
buildmaster: * nmicoud: IDEMPIERE 90
buildmaster: * tbayen: merge with 936777c77efc
buildmaster: * hahmed: IDEMPIERE-595 Swing Login Screen does not show role combo
tbayen: I feel that some differences between zk and swing functionalities are "bugs" that can also be solved in the freeze phase. But I will be careful.
tbayen: And I want definitely webstart working until the release version.
CarlosRuiz: yes tbayen - you can consider zk features not implemented in swing as bugs - I'll take them all
CarlosRuiz: now - I think instead of webstart you must follow the p2 suggested approach
CarlosRuiz: sounds more OSGi and safe
CarlosRuiz: nmicoud_, tbayen, I just committed https://bitbucket.org/idempiere/idempiere/commits/71e511d
CarlosRuiz: please check that I didn't break anything about IDEMPIERE-90 :-)
CarlosRuiz: idea is to make it faster (cache)
CarlosRuiz: gtg now - thanks a lot for the meeting
nmicoud_: i'll have a look, thanks, bye bye