Tryton Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(78)

Issue 66341002: tryton-env: Add bus system

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 2 days ago by nicoe
Modified:
1 day, 5 hours ago
Reviewers:
pokoli, ced, reviewbot
Visibility:
Public.

Description

Add bus system issue7537

Patch Set 1 #

Total comments: 27

Patch Set 2 : Fix remarks #

Total comments: 13
Unified diffs Side-by-side diffs Delta from patch set Stats (+615 lines, -7 lines) Patch
M sao/CHANGELOG View 1 chunk +1 line, -0 lines 0 comments Download
M sao/Gruntfile.js View 1 chunk +2 lines, -1 line 0 comments Download
M sao/index.html View 1 1 chunk +1 line, -0 lines 0 comments Download
A sao/src/bus.js View 1 1 chunk +80 lines, -0 lines 1 comment Download
M sao/src/sao.js View 1 chunk +1 line, -0 lines 0 comments Download
M sao/src/session.js View 3 chunks +7 lines, -1 line 1 comment Download
M tryton/CHANGELOG View 1 chunk +1 line, -0 lines 0 comments Download
A tryton/tryton/bus.py View 1 1 chunk +78 lines, -0 lines 3 comments Download
M tryton/tryton/jsonrpc.py View 1 3 chunks +16 lines, -2 lines 1 comment Download
M tryton/tryton/rpc.py View 1 3 chunks +8 lines, -0 lines 2 comments Download
M trytond/CHANGELOG View 1 chunk +1 line, -0 lines 0 comments Download
A trytond/doc/ref/bus.rst View 1 1 chunk +108 lines, -0 lines 4 comments Download
A trytond/doc/topics/bus.rst View 1 chunk +28 lines, -0 lines 0 comments Download
M trytond/doc/topics/configuration.rst View 1 chunk +26 lines, -0 lines 0 comments Download
M trytond/trytond/backend/database.py View 1 chunk +2 lines, -1 line 1 comment Download
M trytond/trytond/backend/postgresql/database.py View 1 chunk +2 lines, -1 line 0 comments Download
A trytond/trytond/bus.py View 1 1 chunk +244 lines, -0 lines 0 comments Download
M trytond/trytond/config.py View 1 chunk +4 lines, -0 lines 0 comments Download
M trytond/trytond/transaction.py View 1 1 chunk +4 lines, -1 line 0 comments Download
M trytond/trytond/wsgi.py View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 8
reviewbot
https://codereview.tryton.org/66341002/diff/1/trytond/trytond/wsgi.py#newcode155 trytond/trytond/wsgi.py:155: F401 'trytond' imported but unused https://codereview.tryton.org/66341002/diff/1/trytond/trytond/wsgi.py#newcode154 trytond/trytond/wsgi.py:154: E402 module level import not at ...
1 week, 1 day ago (2018-08-10 10:05:21 UTC) #1
ced
1 week ago (2018-08-10 23:12:39 UTC) #2
pokoli
https://codereview.tryton.org/66341002/diff/1/tryton/tryton/rpc.py File tryton/tryton/rpc.py (right): https://codereview.tryton.org/66341002/diff/1/tryton/tryton/rpc.py#newcode126 tryton/tryton/rpc.py:126: notification.LISTEN = True notification should be set to False ...
4 days, 10 hours ago (2018-08-14 07:40:23 UTC) #3
ced
https://codereview.tryton.org/66341002/diff/1/tryton/tryton/notification.py File tryton/tryton/notification.py (right): https://codereview.tryton.org/66341002/diff/1/tryton/tryton/notification.py#newcode1 tryton/tryton/notification.py:1: # This file is part of Tryton. The COPYRIGHT ...
4 days, 9 hours ago (2018-08-14 08:37:02 UTC) #4
nicoe
https://codereview.tryton.org/66341002/diff/1/tryton/tryton/notification.py File tryton/tryton/notification.py (right): https://codereview.tryton.org/66341002/diff/1/tryton/tryton/notification.py#newcode1 tryton/tryton/notification.py:1: # This file is part of Tryton. The COPYRIGHT ...
2 days, 1 hour ago (2018-08-16 16:52:18 UTC) #5
nicoe
Fix remarks
2 days, 1 hour ago (2018-08-16 16:53:25 UTC) #6
reviewbot
https://codereview.tryton.org/66341002/diff/20001/trytond/trytond/wsgi.py#newcode155 trytond/trytond/wsgi.py:155: F401 'trytond' imported but unused https://codereview.tryton.org/66341002/diff/20001/trytond/trytond/wsgi.py#newcode154 trytond/trytond/wsgi.py:154: E402 module level import not at ...
2 days, 1 hour ago (2018-08-16 16:58:04 UTC) #7
ced
1 day, 5 hours ago (2018-08-17 12:55:55 UTC) #8
https://codereview.tryton.org/66341002/diff/1/tryton/tryton/rpc.py
File tryton/tryton/rpc.py (right):

https://codereview.tryton.org/66341002/diff/1/tryton/tryton/rpc.py#newcode39
tryton/tryton/rpc.py:39: return ':'.join('%x' % random.randint(0, 2 ** 32) for _
in range(4))
On 2018/08/16 16:52:17, nicoe wrote:
> On 2018/08/14 08:37:02, ced wrote:
> > Why not use: uuid.uuid4().get_hex()
> 
> Because javascript does not have an easy way to create UUID and I thought that
> it would be good not to make a difference between ids coming from javascript
and
> those from the gtk client.

And what would be the difference?

https://codereview.tryton.org/66341002/diff/20001/sao/src/bus.js
File sao/src/bus.js (right):

https://codereview.tryton.org/66341002/diff/20001/sao/src/bus.js#newcode29
sao/src/bus.js:29: Authorization: 'Session ' + session.get_auth()
missing ending coma

https://codereview.tryton.org/66341002/diff/20001/sao/src/session.js
File sao/src/session.js (right):

https://codereview.tryton.org/66341002/diff/20001/sao/src/session.js#newcode256
sao/src/session.js:256: session.prm.done(function () {
Why not append '.done' after the '.then'

https://codereview.tryton.org/66341002/diff/20001/tryton/tryton/bus.py
File tryton/tryton/bus.py (right):

https://codereview.tryton.org/66341002/diff/20001/tryton/tryton/bus.py#newcode26
tryton/tryton/bus.py:26: return ':'.join('%x' % random.randint(0, 2 ** 32) for _
in range(4))
uuid4

https://codereview.tryton.org/66341002/diff/20001/tryton/tryton/bus.py#newcode68
tryton/tryton/bus.py:68: logger.info(str(error))
Should it not be an logger.error?

https://codereview.tryton.org/66341002/diff/20001/tryton/tryton/bus.py#newcode71
tryton/tryton/bus.py:71: logger.info(str(error))
I think it should be a logger.debug because it is expected to happen.

https://codereview.tryton.org/66341002/diff/20001/tryton/tryton/jsonrpc.py
File tryton/tryton/jsonrpc.py (right):

https://codereview.tryton.org/66341002/diff/20001/tryton/tryton/jsonrpc.py#ne...
tryton/tryton/jsonrpc.py:362: scheme + '://' + self._host + ':' +
str(self._port),
I think the _host name must be encoded.

https://codereview.tryton.org/66341002/diff/20001/tryton/tryton/rpc.py
File tryton/tryton/rpc.py (right):

https://codereview.tryton.org/66341002/diff/20001/tryton/tryton/rpc.py#newcode89
tryton/tryton/rpc.py:89: url = CONNECTION.url + '/bus'
In sao, it is bus that construct the URL. I think it is a better separation
because it keeps all the bus details inside bus.py

https://codereview.tryton.org/66341002/diff/20001/tryton/tryton/rpc.py#newcode90
tryton/tryton/rpc.py:90: listener = threading.Thread(
Is it not better that bus.py start the thread?

https://codereview.tryton.org/66341002/diff/20001/trytond/doc/ref/bus.rst
File trytond/doc/ref/bus.rst (right):

https://codereview.tryton.org/66341002/diff/20001/trytond/doc/ref/bus.rst#new...
trytond/doc/ref/bus.rst:18: A list of dictionaries. The messages that should be
handled by the client.
no more a list

https://codereview.tryton.org/66341002/diff/20001/trytond/doc/ref/bus.rst#new...
trytond/doc/ref/bus.rst:35: .. classmethod:: Bus.publish(message, user,
bus_session)
As we discussed for the chat, we will need to replace user and bus_session by a
generic channel.

https://codereview.tryton.org/66341002/diff/20001/trytond/doc/ref/bus.rst#new...
trytond/doc/ref/bus.rst:46: .. classmethod:: Bus.subscribe(database, user,
bus_session[, client_timestamp])
Idem, the user and bus_session should be a list of channels.
The user channel must be deducted from the authentication.

https://codereview.tryton.org/66341002/diff/20001/trytond/doc/ref/bus.rst#new...
trytond/doc/ref/bus.rst:71: .. method:: notify(title[, body[, icon[, priority[,
user[, session]]]]])
user and session should be replaced by channel

https://codereview.tryton.org/66341002/diff/20001/trytond/trytond/backend/dat...
File trytond/trytond/backend/database.py (right):

https://codereview.tryton.org/66341002/diff/20001/trytond/trytond/backend/dat...
trytond/trytond/backend/database.py:223: def has_channel(cls):
I think it can depend on the version of the database.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 3870a69