32501: wrong fix proposed (removes "off" part)

[bagage]

Example

Osmose suggests to replace:

- opening_hours = Mo off, Tu-Th 09:00-18:00; Fr 09:00-19:00; Sa 08:00-18:00; Su off 
+ opening_hours = Mo    , Tu-Th 09:00-18:00; Fr 09:00-19:00; Sa 08:00-18:00; Su off 

But this is invalid (eg they have not the same meaning) if I compare current and proposed versions.

[frodrigo]

cc @dfaure

[dfaure-kdab]

That looks wrong indeed, but I don't understand: I can't reproduce this locally in the docker setup from osmose-backend git master...

I apply http://www.davidfaure.fr/2021/add_test.diff and then

docker-compose -f docker-compose.yml -f docker-compose-dev.yml build
docker-compose -f docker-compose.yml -f docker-compose-dev.yml run backend bash

Then in that shell I run pytest -k Opening which says 1 passed

Any idea why this works differently from the real site?

[frodrigo]

@jocelynj are we using the last version ?

[jocelynj]

Backends are currently on version 0176240a4ce437a0fb5fbf512b259781e190f5b5. Should I update?

In fact, you are talking about openinghours. Requirements.txt points to version 74bd4c5. I will check what we have on backends.

I see this:

$ pip3 show PyKOpeningHours
Name: PyKOpeningHours
Version: 1.4.0
Summary: Validator for OSM opening_hours expressions
Home-page: UNKNOWN
Author: David Faure
Author-email: UNKNOWN
License: AGPL
Location: /data/project/osmose/.local/lib/python3.7/site-packages
Requires: 
Required-by: 

And a pip3 install -r requirements.txt recompiles PyKOpeningHours, but doesn't update the version. Do I need to remove this version before installing it?

[frodrigo]

Maybe because it is already up to date ?

[dfaure-kdab]

Yep this hasn't changed in a long time. Could you maybe try the unittest addition on the server?

[jocelynj]

I've applied your test patch on osm211, and launched the test, and it is failing:

>       assert not a.node(None, {'opening_hours': 'Mo off, Tu-Th 09:00-18:00; Fr 09:00-19:00; Sa 08:00-18:00; Su off'})
E       AssertionError: assert not {'class': 32501, 'fix': {'opening_hours': 'Mo,Tu-Th 09:00-18:00; Fr 09:00-19:00; Sa 08:00-18:00; Su off'}, 'subclass': 0}
E        +  where {'class': 32501, 'fix': {'opening_hours': 'Mo,Tu-Th 09:00-18:00; Fr 09:00-19:00; Sa 08:00-18:00; Su off'}, 'subclass': 0} = <bound method TagFix_Opening_Hours.node of <plugins.TagFix_Opening_Hours.TagFix_Opening_Hours object at 0x7f54441ae048>>(None, {'opening_hours': 'Mo off, Tu-Th 09:00-18:00; Fr 09:00-19:00; Sa 08:00-18:00; Su off'})
E        +    where <bound method TagFix_Opening_Hours.node of <plugins.TagFix_Opening_Hours.TagFix_Opening_Hours object at 0x7f54441ae048>> = <plugins.TagFix_Opening_Hours.TagFix_Opening_Hours object at 0x7f54441ae048>.node

Could you add a test on a Pull-Request, so that we can also check Github CI result?

[dfaure]

Thanks for the test. Very interesting. Pull request done in https://github.com/osm-fr/osmose-backend/pull/1307

[jocelynj]

The test is now working on osm211, where I did the following:

pip3 uninstall PyKOpeningHours
pip3 install -r requirements.txt
pytest -k Opening

It looks like it didn't update the version last time.

[jocelynj]

I've launched the same commands on all backends handled by OSM-Fr.

Can you check if it fixes the original issue?

[dfaure-kdab]

The "Example" link in the bug report points to a JSON error now, I guess because the error on that node no longer exists. In other words, I think the original issue is fixed, yes.

[jocelynj]

Thanks for the confirmation.