0032681: Provide a webservice which returns the offline/online status of a store

Openbravo Issue Tracking System - Retail Modules
View Issue Details

ID	Project	Category	View Status	Date Submitted	Last Update
0032681	Retail Modules	Web POS	public	2016-04-15 17:32	2016-05-18 16:38

Reporter	mtaal
Assigned To	mtaal
Priority	normal	Severity	minor	Reproducibility	have not tried
Status	closed	Resolution	fixed
Platform		OS	5	OS Version
Product Version
Target Version	RR16Q3	Fixed in Version	RR16Q3
Merge Request Status
Review Assigned To	AugustoMauch
OBNetwork customer	No
Support ticket
Regression level
Regression date
Regression introduced in release
Regression introduced by commit
Triggers an Emergency Pack	No

Summary	0032681: Provide a webservice which returns the offline/online status of a store
Description	The webservice should be available on the central and store server. On the store server it can return its current status from the mobile server table. From the central server it should check the mobile server status table and check how old the last update there is. If it is not too old (configurable preference) then the status from the table should be returned. If too old then the store server should be called with the same webservice to retrieve the current online status. The webservice should be documented in the store server wiki pages.
Steps To Reproduce	See description
Proposed Solution
Additional Information
Tags	No tags attached.
Relationships
Attached Files

Issue History
Date Modified	Username	Field	Change
2016-04-15 17:32	mtaal	New Issue
2016-04-15 17:32	mtaal	Assigned To	=> mtaal
2016-04-15 17:32	mtaal	OBNetwork customer	=> No
2016-04-15 17:32	mtaal	Triggers an Emergency Pack	=> No
2016-04-20 09:23	mtaal	Note Added: 0085765
2016-04-21 08:12	hgbot	Checkin
2016-04-21 08:12	hgbot	Note Added: 0085809
2016-04-21 08:12	hgbot	Status	new => resolved
2016-04-21 08:12	hgbot	Resolution	open => fixed
2016-04-21 08:12	hgbot	Fixed in SCM revision	=> http://code.openbravo.com/erp/pmods/org.openbravo.mobile.core/rev/490a2e8132e21f1cfd205e01598ddc2825fd3393 [^]
2016-04-21 08:12	hgbot	Checkin
2016-04-21 08:12	hgbot	Note Added: 0085810
2016-05-03 14:52	mtaal	Review Assigned To	=> AugustoMauch
2016-05-15 09:26	mtaal	Note Added: 0086449
2016-05-16 00:55	hgbot	Checkin
2016-05-16 00:55	hgbot	Note Added: 0086453
2016-05-18 16:38	AugustoMauch	Note Added: 0086555
2016-05-18 16:38	AugustoMauch	Status	resolved => closed
2016-05-18 16:38	AugustoMauch	Fixed in Version	=> RR16Q3

Notes

Code review comments and reaction, next push will contain changes:

>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
I finally did the code review of https://issues.openbravo.com/view.php?id=32681. [^] I have a few comments:

1) Three methods (executeServerRequest, executeCentralRequest, executeRequest) have this comment:
* This method performs so-called store server state handling. If the calling server can not be
* reached then this server can decide to go offline this depends on the
* {@link MobileServerDefinition#isTriggerstate()} value of the called server.

But at no point they set the status of the server to false if the request fails, I think.
MT>> is done through the tryConnection used by all three of them. I adapted the comment to reflect this.

2) There is a lot of duplicated code in the executeServerRequest and executeCentralRequest methods, it could make sense to refactor them.
MT>> indeed! I refactored this
3) The getCentralServer method adds this (MobileServerDefinition.PROPERTY_MOBILESERVERKEY + "!='" + thisServerDef.getMobileServerKey()) to the where clause, that results in the method returning null if called from the central server.
MT>> Yes is on purpose, I added this to the comment. The reason is that this method is used to call the central server. If a server starts doing requests to itself then we will get infinite loops which are really hard to detect. I added this as a comment.

4) Shouldn't that method be placed in the MobileServerController class? That class has similar methods, like getThisServerDefinition. The same applies for the getMobileServer method.
MT>> yes changed it
5) Wrong log here.
MT>> Changed
6) Regarding the getMobileServerForStore method. Why returning only the first store server that applies? Isn't it a little arbitrary? Also I did not see that the method was used anywhere in the changesets related with the issue.
MT>> removed it
7) Removing the getRetriesPreference is an API change (I think those are not checked automatically in retail).
MT>> it was introduced in this release so not yet in official api, so can be removed without breaking api
8) Duplicated code in StatusUpdateRunnable.run(). It would be better if that method called getMobileServer. Maybe it would make sense to cache the mobile server definitions if the method is called often enough.
MT>> yes changed to call getMobileServer
9) Just out of curiosity, what locking problems happened in this code? Not saying that is wrong, only that I would not have been able to detect them myself.
MT>> The status is updated for each request, there can be many simultaneous requests, and they will update a server def at the same time in maybe somewhat longer transactions. So there is a risk that multiple threads will update the mobile server table.