Implemented qvarn notification feature #4
Conversation
https://raw.githubusercontent.com/vaultit/qvarn/master/docs/qvarn-api-doc/050-api-overview.yarn - Updated .gitignore to ignore Eclipse project files - Makefile has optional PYTHON_3.6 parameter and code for setting up database - Implemented notifications view - Implemented notifications view tests - Notifications related updated to postgresql backend
|
Tests were implemented using the qvarn documentation example and run successful against the notifications view |
There was a problem hiding this comment.
In general looks good, but there are some things, that probably could be fixed.
listen_on_type probably should not bu duplicated in table and json blob, unless there is a good reason for that.
User information is not added to changes table.
I'm not 100% sure, but probably it might be possible to do an SQL or at least a JSON injection, when using op method and raw JSON strings. PostgreSQL has methods that allows you to avoid dealing with JSON serialization directly.
And I don't like style of tests in general. This is my personal opinion, but I like, when tests have almost no logic, they just call a function and check the results. It is OK to repeat things in tests. Tests should be focused on testing one simple thing. In you case, one test for creating listeners tests all different variations, how listener can be created.
An one more serious issue with tests is that they share a global state. This makes it impossible to run one test function.
qvarn/backends/postgresql.py
Outdated
| @@ -6,13 +6,18 @@ | |||
| import os | |||
| import pathlib | |||
| import urllib.parse | |||
| import datetime | |||
There was a problem hiding this comment.
Correct. Oversaw it due to my linter not working.
qvarn/backends/postgresql.py
Outdated
|
|
||
| import ruamel.yaml as yaml | ||
| import sqlalchemy as sa | ||
| from sqlalchemy.engine import reflection | ||
| from sqlalchemy.dialects.postgresql import JSONB | ||
| from sqlalchemy.dialects.postgresql import ARRAY | ||
| from sqlalchemy.dialects.postgresql import insert | ||
| from sqlalchemy.ext.compiler import compiles | ||
| from sqlalchemy.sql.functions import FunctionElement | ||
| from sqlalchemy.sql.expression import exists |
There was a problem hiding this comment.
Same here. Only exists is unused and remained from a test.
qvarn/backends/postgresql.py
Outdated
| @@ -22,11 +27,21 @@ | |||
| from qvarn.backends import WrongRevision | |||
| from qvarn.backends import UnexpectedError | |||
| from qvarn.validation import validated | |||
| from coreschema.schemas import Boolean | |||
There was a problem hiding this comment.
Unused import. I think you should used a static code checker, something like flake8.
There was a problem hiding this comment.
Yes you're absolutely right. Eclipse does code analysis, but looking again it seems that the additional Eclipse plugin I installed to adhere the editorconfig file messed with the PyDev code analysis.
qvarn/backends/postgresql.py
Outdated
| @@ -109,6 +124,7 @@ def __init__(self, name, values, inlist): | |||
| self.values = values | |||
| self.inlist = inlist | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
According to PEP-8, methods should be separated by a single blank line:
Method definitions inside a class are surrounded by a single blank line.
There was a problem hiding this comment.
Yes. Once again my pep8 automatic error reporting seem to not run due to some problems with the project setup. Personally I'm also no fan of having a 1 line rule for classes and 2 lines for top-level functions. I find it not very consistent and makes reading classes more difficult.
qvarn/backends/postgresql.py
Outdated
|
|
||
| def _create_resource_type_tables(self, schema): | ||
| version = schema['versions'][-1] | ||
| subpaths = version.get('subpaths', {}) | ||
| resource_type = schema['type'] | ||
| files = version.get('files', []) | ||
|
|
||
There was a problem hiding this comment.
Hanging trailing spaces does not contribute to the code, but make it more difficult to review.
tests/views/test_notifications.py
Outdated
| def test_listeners_post (client, storage): | ||
| global LISTENER_1, LISTENER_2, LISTENER_3, LISTENER_4, REVISION_1 | ||
|
|
||
| storage.wipe_all_data('test') |
There was a problem hiding this comment.
You are wiping test resources, but in tests, you are using orgs resource.
| assert result.status_code == 201 | ||
| assert result.headers['location'].endswith(f'/orgs/listeners/{listener["id"]}') | ||
| assert listener['type'] == 'listener' | ||
| assert 'id' in listener |
There was a problem hiding this comment.
I think, this already was checked in 56th line.
| LISTENER_3 = listener['id'] | ||
| assert 'notify_on_all' in listener and listener['notify_on_all'] is True | ||
| elif k == 'listener4': | ||
| LISTENER_4 = listener['id'] |
There was a problem hiding this comment.
I think, this tests contains too much logic and that makes it more difficult to read and debug. I like, when tests are simple and focused on one thing.
tests/views/test_notifications.py
Outdated
| result = client.get('/orgs/listeners') | ||
| listeners = {r['id']:None for r in result.json()['resources']} | ||
|
|
||
| assert 'resources' in result.json() |
There was a problem hiding this comment.
This is already checked in 96th line.
tests/views/test_notifications.py
Outdated
|
|
||
| # Test if listeners are listed | ||
| result = client.get('/orgs/listeners') | ||
| listeners = {r['id']:None for r in result.json()['resources']} |
There was a problem hiding this comment.
You can simply use set() here, instead of dict().
- Changes and fixed to adhere to PEP8 - Removing unused imports
Implemented qvarn notification feature based on https://raw.githubusercontent.com/vaultit/qvarn/master/docs/qvarn-api-doc/050-api-overview.yarn
database