[bukuserver API] improve tag replacement/deletion

[2ynn]

Addresses most comments made by @LeXofLeviafan in issue #672

1. ApiTagView.put() (PUT /api/tags/<tag>) now expects a list of strings as arguments instead of a comma-separated string. Passing an empty list results in bad response 400.

2. A separate ApiTagView.delete() method was added to delete an existing tag (DELETE /api/tags/<tag>).

3. A trailing slash is now permitted on GET /api/tags/ route.

ORIGINAL DESCRIPTION
_0. Clarify the expected data type received in ApiTagView.put:
PUT /api/tags/ now returns 400 if no string is passed as input data

1. Allow ApiTagView.put() to update an existing tag with multiple ones by passing a single, comma-separated string. Instead of modifying the behavior of replace_tag(), which in my opinion would come with too many side effects, I have decided to wrap the new_tags comma-separated string into a list, in the same way as it is done in the TagModelView.update_model() method.
2. Allow tag deletion through passing an empty string to ApiTagView.put.
   Passing {tags: ''} as data to PUT /api/tags/<tag> now makes it possible to delete an existing tag.
   The delete_tag_at_index() function is now called with chatty=False to bypass confirmation. To do so, an additional optional argument chatty had to be added to the replace_tag() function.
3. Allow trailing slashes in URLs by setting strict_slashes=False to all api endpoints
4. Unrelated but test_bookmark_range_api was not passing on master due to bookmark titles not matching between parametrization and assertion._

[LeXofLeviafan]

To clarify:

Instead of modifying the behavior of replace_tag(), which in my opinion would come with too many side effects, I have decided to wrap the new_tags comma-separated string into a list

It's better to make the input format straightforward instead of working around a tricky one whenever you use the function. A simple "accepting a list & processing it as-is" is a much better behaviour for a function API than "accepting a list and mashing its contents together in a way that takes effort to describe".

Passing {tags: ''} as data to PUT /api/tags/<tag> now makes it possible to delete an existing tag.

Since this API appears to be going for RESTful approach, it's probably better to make deletion a separate operation, instead of combining it with edit operation. (…The reasoning is the same here, come to think of it: web/library API doesn't have the same limitations as CLI so there's no need to complicate it by imposing them anyway.)

P.S. You may want to amend existing commits & force-push your changes instead of adding new commits; the repo owner prefers to avoid having multiple small commits (but he doesn't want to use squash-merge).

[jarun]

@LeXofLeviafan can you have a look at the build failure on master?

[LeXofLeviafan]

@jarun the only thing that fails to pass on current master is pylint (due to having else on try blocks with catch blocks ending on returns).
Maybe it should be moved into a separate check (along with flake8); I doubt that it'd be much different between Python versions, and the result would be making clear whether it's linting or tests that doesn't pass.

[jarun]

pylint (due to having else on try blocks with catch blocks ending on returns)

Why can't we remove the else? What is this syntax try ... else? Why can't it be avoided?

[LeXofLeviafan]

@2ynn try doing a rebase on upstream/master – the current master is passing all checks.

[2ynn]

@2ynn try doing a rebase on upstream/master – the current master is passing all checks.

Hi @LeXofLeviafan, I checked out the upstream/master and I still get the same error on test_bookmark_range_api when I run pytest -vv tests_server.py:

client = <FlaskClient <FlaskAPI 'bukuserver.server'>>

    def test_bookmark_range_api(client):
        status_code = 200
        kwargs_list = [
            {"data": {'url': 'http://google.com'}},
            {"data": {'url': 'http://example.com'}}]
        for kwargs in kwargs_list:
            rd = client.post('/api/bookmarks', **kwargs)
            assert rd.status_code == status_code
        rd = client.get('/api/bookmarks/1/2')
        assert rd.status_code == status_code
>       assert rd.get_json() == {
            'bookmarks': {
                '1': {'description': '', 'tags': [], 'title': 'Google', 'url': 'http://google.com'},
                '2': {'description': '', 'tags': [], 'title': 'Example Domain', 'url': 'http://example.com'}}}
E       AssertionError: assert {'bookmarks': {'1': {'description': '', 'tags': [], 'title': 'Google', 'url': 'http://google.com'}, '2': {'description': '', 'tags': [], 'title': None, 'url': 'http://example.com'}}} == {'bookmarks': {'1': {'description': '', 'tags': [], 'title': 'Google', 'url': 'http://google.com'}, '2': {'description': '', 'tags': [], 'title': 'Example Domain', 'url': 'http://example.com'}}}
E         Differing items:
E         {'bookmarks': {'1': {'description': '', 'tags': [], 'title': 'Google', 'url': 'http://google.com'}, '2': {'description': '', 'tags': [], 'title': None, 'url': 'http://example.com'}}} != {'bookmarks': {'1': {'description': '', 'tags': [], 'title': 'Google', 'url': 'http://google.com'}, '2': {'description': '', 'tags': [], 'title': 'Example Domain', 'url': 'http://example.com'}}}
E         Full diff:
E           {
E            'bookmarks': {'1': {'description': '',
E                                'tags': [],
E                                'title': 'Google',
E                                'url': 'http://google.com'},
E                          '2': {'description': '',
E                                'tags': [],
E         -                      'title': 'Example Domain',
E         ?                               ^^^^^^^ --------
E         +                      'title': None,
E         ?                               ^^^
E                                'url': 'http://example.com'}},
E           }

tests/test_server.py:209: AssertionError

[LeXofLeviafan]

…I've looked it over, and found that the cause here is likely twofold:

• the test is intended specifically for the web API range query, but it uses other web API operations (instead of doing environment setup directly); the test fails because the db setup section produces unexpected result
• the default behaviour for the "add bookmark" operation is "fetch missing data from the Internet" – not something you'd want done by default in the code, and especially in the tests (in the code you'd want to activate this explicitly, and in a test you'd want to mock every such network request if you need it it at all); the reason the test fails on your machine is because it's doing something that no test should really ever do: interact with entities outside the test environment (live network, in this case)

In other words, what should be changed isn't the value check but the db setup section.

(…As for exact reason why you're not getting back a webpage when fetching http://example.com, I couldn't tell, but it shouldn't be something that matters when running a test.)

[2ynn]

Hi @LeXofLeviafan, thanks for your review and sorry again for disappearing :/
I just pushed some updates to my PR:

1. added del_tags boolean field to ApiBookmarkRangeEditForm. By default del_tags is set to False,
   meaning that the list of tags provided will be appended to the existing tags (i.e. + is prepended to the comma-separated list). Passing del_tags: True along with the request will instead prepend -. del_tags can be set independently for each record.
2. added more detailed error messages

[LeXofLeviafan]

You may want to run the tests locally (possibly on freshly reinstalled venv if you're not using tox to run them) to make sure your branch passes CI checks.

[LeXofLeviafan]

These and the nonexistent-variable one are causing the flake8 check to fail.

The unit tests are failing on master as well, so I reported a bug regarding the error (#698).

[LeXofLeviafan]

The unit tests are failing on master as well, so I reported a bug regarding the error (#698).

Tests were fixed in upstream; your branch should be passing once you rebase and fix flake8 errors.

[LeXofLeviafan]

…Incidentally, here's a quote from the very first comment:

P.S. You may want to amend existing commits & force-push your changes instead of adding new commits; the repo owner prefers to avoid having multiple small commits (but he doesn't want to use squash-merge).

(Multiple commits can be squashed together using soft-reset + amend)

[jarun]

@LeXofLeviafan please merge when this is ready.

[LeXofLeviafan]

…Merging as requested.

[LeXofLeviafan]

…Incidentally, I suggest referencing the issue in commit name next time (not the pull-request) 😅

Also linking the pull-request to the issue is generally a good idea (happens automatically if its description is worded accordingly).