GPII-1311: OLB should use WebSockets handler #415
Conversation
I couldn't check whether the test run (I found no way to make them run separately from the whole test suite, and I could not figure out how to add them to the exisiting test suite: whatever I did, the total number of tests remained the same).
|
Hey @cstrobbe, thanks for being so responsive! Just one thing, and sorry if I explained it wrong during our meeting today. As part of this pull request, we want to allow web-solutions to receive settings from a local running instance of the flowManager through the browserChannel. Because of this, we need to add the settingsHandlers block in the same way you already did but into the web solutions registry instead. Yes, it's true that I was testing things by having the solution entry in a platform's solutions registry, but if I'm not wrong, this is the way we want to go for the review, exactly in the same way I did in this pull request for the cloud4all site. |
| "conf": { | ||
| "type": "gpii.settingsHandlers.webSockets", | ||
| "options": { | ||
| "path": "gpii.eu/olb/" |
There was a problem hiding this comment.
the path must be the same as the solution id (eu.gpii.olb), short explanation: this is the way for both the browserChannel and the webSockets settingsHandler to know who and where their preferences are stored.
There was a problem hiding this comment.
Oh, I see. I misread something that I had seen in your pull request for the Cloud4all site. Sorry.
This commit makes the OLB (in the web solutions registry) use the webSockets settings handler instead of the noSettings settings handler.
This commit makes the OLB (in the web solutions registry - now for real) use the webSockets settings handler instead of the noSettings settings handler.
|
Hi @javihernandez , Anyway, this pull request should now be ready for another review. |
Yeah, as I said, I was testing this with a platform's solutions registry entry (linux in my case), but probably I didn't stress that we were expecting to have them in the web solutions registry entry. |
At all, with the web one should be enough. In any case, we still need to figure out how to test this new feature, but for now, I'd say this is probably ready to go into review4. (after reviewing it) |
|
So there isn't much point in including OLB in the win32 registry unless I define a way to start it (in Chrome). Should I remove it from win32.json? |
Sure, you can remove it from the pull request |
OLB had been added to win32.json due to a misunderstanding.
|
OLB has now been removed from win32.json. |
|
Hey christophe, I think the conflict is just due to the web.json file has been modified both in this pull and in review 4 branch. If you feel like resolving it, you can merge your branch with the latest version of review4.. that should resolve it.. Else I can resolve while reviewing. The pull request looks good with the exception of tests still being located in the windows folder.. Could you move them to the tests/platform/cloud (https://github.com/GPII/universal/tree/master/tests/platform/cloud) folder instead. And then instead of listing the test file in index-windows.js, you can put it in the tests/all-tests.json (https://github.com/GPII/universal/blob/master/tests/all-tests.js#L29-L39). Other than that, it looks ready to into review 4 |
info.cloud4all.www was added to the review4 branch after I created my pull request. (Doing this through the web interface, since I made a mess of my local branch.)
|
I have removed the tests from the Windows folder; the tests for tests/platform/cloud already existed before this pull request, so GitHub still complains about conflicts; this may be due to the lines https://github.com/GPII/universal/blob/review4/testData/solutions/web.json#L610-L616 ( |
cstrobbe
changed the title
|
✨ |
This pull request adds OLB to the Windows registry. This is mostly copied from the web.json registry, with a few differences:
I couldn't check whether the tests run (I found no way to make them run separately from the whole test suite, and I could not figure out how to add them to the existing test suite: whatever I did, the total number of tests remained the same).