QGEP / qgepqwat2ili

3 stars 3 forks source link

2022-12 Change filter wastewater structures #94

Open sjib opened 1 year ago

sjib commented 1 year ago
sjib commented 1 year ago

Should fix https://github.com/QGEP/qgepqwat2ili/issues/93

sjib commented 1 year ago

Second wastewater_node now ok, but reach point of overflow pipe at wrong place: 20221215_haltung_ueberlauf_von_hp_falscher_ort

20221215_haltung_ueberlauf_von_hp_falsche_ak_ref and wrong relation to ak instead of overflow ak

ponceta commented 1 year ago

@olivierdalang will review this.

olivierdalang commented 1 year ago

@sjib Could you add a regression test (a test that would fail without your fix, and that works with your fix) ? You can copy test_case_e_export_selection in test_qgep.py to TestRegressions, adapt the selected ids as well as the assertions in the output (the tests use the demo data, so you can just use whatever you used when you manually reproduced the issue).

If you're not sure how this can be done, can you give me an example of an object id to select that creates an export with missing objects, also stating the ids of the missing objects in the export ?

Once that's in place, I will suggest some refactoring directly in the code:

(it's better to do the regression test first, so we can ensure your fix will still work after refactoring)

sjib commented 1 year ago

I cannot find an example in the testdataset Arbon at the moment. I have to check back on the data with the issue. The integration of additional datamodels in Export / Import has higher priority.

olivierdalang commented 1 year ago

(converting this to draft so we don't merge by accident until comment are sorted)

sjib commented 1 year ago

Should solve: [Additional wastwater_nodes not exported with Limit to selection

93](https://github.com/QGEP/qgepqwat2ili/issues/93)

ponceta commented 1 year ago

@sjib do we want this for 1.6.1 or do we save it for later?

What has to be achieved to finish this work?

sjib commented 1 year ago

@sjib do we want this for 1.6.1 or do we save it for later?

What has to be achieved to finish this work?

The main point that Olivier suggested was to write a regression test. I tried, but I was not sucessful to find a suitable example in our demodata. So if this is the only missing part for 1.6.1 I suggest not to merge it for now, but still include this point in testing, if all wastewater_nodes will get exported (see original issue: "Additional wastwater_nodes not exported with Limit to selection"

93).

We then would have a reconfirmation of the need during testing (here especially include @DflGruBoe ) and could still integrate it in the final version.