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

Notes
(0085765) mtaal (viewer) 2016-04-20 09:23	Docs: http://wiki.openbravo.com/wiki/Store_Server_Webservices#Retrieve_Store_Server_Status [^]

(0085809) hgbot (developer) 2016-04-21 08:12	Repository: erp/pmods/org.openbravo.mobile.core Changeset: 490a2e8132e21f1cfd205e01598ddc2825fd3393 Author: Martin Taal <martin.taal <at> openbravo.com> Date: Wed Apr 20 09:29:26 2016 +0200 URL: http://code.openbravo.com/erp/pmods/org.openbravo.mobile.core/rev/490a2e8132e21f1cfd205e01598ddc2825fd3393 [^] Fixes issue 32681: Provide a webservice which returns the offline/online status of a store Adds webservice and other improvements for updating the status --- M src-db/database/sourcedata/AD_REF_LIST.xml M src/org/openbravo/mobile/core/process/JSONProcessSimple.java M src/org/openbravo/mobile/core/servercontroller/MobileServerController.java M src/org/openbravo/mobile/core/servercontroller/MobileServerRequestExecutor.java M src/org/openbravo/mobile/core/servercontroller/MobileServerUtils.java ---

(0085810) hgbot (developer) 2016-04-21 08:12	Repository: erp/pmods/org.openbravo.mobile.core Changeset: b1b8b84cfabb57bc4799600cfa13a098be1f2e57 Author: Martin Taal <martin.taal <at> openbravo.com> Date: Wed Apr 20 15:48:48 2016 +0200 URL: http://code.openbravo.com/erp/pmods/org.openbravo.mobile.core/rev/b1b8b84cfabb57bc4799600cfa13a098be1f2e57 [^] Related to issue 32681: Provide a webservice which returns the offline/online status of a store Added missing classes --- A src/org/openbravo/mobile/core/servercontroller/MobileServerStatusUpdateHandler.java A src/org/openbravo/mobile/core/servercontroller/RetrieveMobileServerStatus.java ---

(0086449) mtaal (viewer) 2016-05-15 09:26	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.

(0086453) hgbot (developer) 2016-05-16 00:55	Repository: erp/pmods/org.openbravo.mobile.core Changeset: dc7dba5a385b425f3d6fe2741beedd3e85ed2c03 Author: Martin Taal <martin.taal <at> openbravo.com> Date: Sun May 15 09:32:20 2016 +0200 URL: http://code.openbravo.com/erp/pmods/org.openbravo.mobile.core/rev/dc7dba5a385b425f3d6fe2741beedd3e85ed2c03 [^] Related to issue 32681: Provide a webservice which returns the offline/online status of a store Changes for code review MobileCoreLoginUtilsServlet.java: do preference reading in utils class MobileServerController.java: several new utility methods used by webservice call MobileServerRequestExecutor.java: updated comments to reflect inner working of methods, refactor to prevent code duplication MobileServerStatusUpdateHandler.java: use utility method to prevent code duplication MobileServerUtils.java: move server-related methods to MobileServerController class --- M src/org/openbravo/mobile/core/login/MobileCoreLoginUtilsServlet.java M src/org/openbravo/mobile/core/servercontroller/MobileServerController.java M src/org/openbravo/mobile/core/servercontroller/MobileServerRequestExecutor.java M src/org/openbravo/mobile/core/servercontroller/MobileServerStatusUpdateHandler.java M src/org/openbravo/mobile/core/servercontroller/MobileServerUtils.java ---

(0086555) AugustoMauch (administrator) 2016-05-18 16:38	Code reviewed and verified in [1]@dc7dba5a385b [1] https://code.openbravo.com/erp/pmods/org.openbravo.mobile.core [^]

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

Relationships [ Relation Graph ] [ Dependency Graph ]