search

georchestra / mapstore2-cadastrapp

repository for the mapstore2 version of cadastrapp
GNU General Public License v3.0
2 stars 10 forks source link

wrong query for batiments on a plot without batiments #168

Closed landryb closed 2 years ago

landryb commented 2 years ago

trying cadastrapp v2.0 backend (from https://github.com/georchestra/cadastrapp/commit/abc6ea5) with ms2-cadastrapp from current HEAD (eg https://github.com/georchestra/mapstore2-cadastrapp/commit/3cab1ba), selecting a plot without batiments, if i open the information form, i see in the network console that a wrong query is done to /cadastrapp/services/getBatiments?parcelle=630044000ZH0013

the backend replies with a code 400 and message Required request parameter 'dnubat' for method parameter type String is not present

going to the 'batiment' tab in the form shows an endlessly spinning spinner..

image

landryb commented 2 years ago

if i do the query on a plot with a building (eg /cadastrapp/services/getBatimentsByParcelle?parcelle=630044000ZH0141 returns [{"dnubat": " A"}] i see two requests done:

so, to fix this issue:

cc @catmorales

landryb commented 2 years ago

@tdipisa can you please tell my why you removed your assignment ? Is this issue triaged/being worked on ? Thanks.

tdipisa commented 2 years ago

@tdipisa can you please tell my why you removed your assignment ? Is this issue triaged/being worked on ? Thanks.

@landryb it seems this issue is referring the new Cadastrapp backend while our implementation of the ms2-cadastrapp plugin is based on the older version. I think this issue should be included in a new order @catmorales also because it relates to a feature we have released as part of the first delivery of the MS Cadastrapp plugin (marché subséquent N° 2) for which the warranty period is expired for a while now.

landryb commented 2 years ago

i'm pretty sure if you (at least) try to replicate with the old cadastrapp backend you'll see the issue - sadly right no it seems https://georchestra.geo-solutions.it/mapstore/#/context/urbanismecadastrapp seems broken as trying to select a plot yields no selection in the cadastrapp pane.

And i'm very sorry, but no "our implementation is based on the older version of the backend" isn't an acceptable justification for ignoring this issue, which i'm reporting on master because i'm experiencing it with ms2-cadastrapp master.

tdipisa commented 2 years ago

I'm sorry @landryb but I am not ignoring anything since I am here to answer you. It's good that you have opened this issue here, of course, since you have found a problem testing this extension with the Cadastrapp backend v2.0. I'm just saying/explaining that GeoSolutions has implemented this extension before of that new version of the backend. Therefore we cannot ensure it is fully compatible with it if we don't spend time to check and provide needed updates, that isn't a task or something that has been commissioned to us so far. This is the only reason why I've removed my assignment, that's it. I'm anyway happy and available to plan an update of the cadastrapp extension if/when needed, of course.

tdipisa commented 2 years ago

@landryb

i'm pretty sure if you (at least) try to replicate with the old cadastrapp backend you'll see the issue - sadly right no it seems https://georchestra.geo-solutions.it/mapstore/#/context/urbanismecadastrapp seems broken as trying to select a plot yields no selection in the cadastrapp pane.

For this maybe you can specify better the steps to reproduce on our DEV? It is not clear to me the selection I have to do to reproduce the problem. If there is a regression depending on the last update we have provided within the contract 3, we can provide a fix.

landryb commented 2 years ago

@landryb

i'm pretty sure if you (at least) try to replicate with the old cadastrapp backend you'll see the issue - sadly right no it seems https://georchestra.geo-solutions.it/mapstore/#/context/urbanismecadastrapp seems broken as trying to select a plot yields no selection in the cadastrapp pane.

For this maybe you can specify better the steps to reproduce on our DEV? It is not clear to me the selection I have to do to reproduce the problem. If there is a regression depending on the last update we have provided within the contract 3, we can provide a fix.

Well, yesterday it didnt work (eg clicking on a plot didn't yield any selection), today it does - but since i cant view building properties (no login with enough rights) i can't try to reproduce the issue on this instance.

landryb commented 2 years ago

ok, spent a bit more time on this issue and done a bit more investigations to see if it's a regression in the frontend or backend, the issue can be easily reproduced when opening the info form for a plot which doesn't contain buildings (eg /cadastrapp/services/getBatimentsByParcelle?parcelle=630044000ZH0141 returns an empty array):

i can reproduce the issue with curl against both backends, 1.9.3 replies with a 200 code and an empty array while 2.0 replies with a 400 code.

so in a sense, yes it's a regression from running against the last version of the backend (which is more strict wrt missing parameters ?), but in the first place ms2-cadastrapp wasn't supposed to call getBatiments without dnubat (as it's been doing also before latest changes in ms2-cadastrapp) - the getBatiments method in https://github.com/georchestra/cadastrapp/blob/master/cadastrapp/src/main/java/org/georchestra/cadastrapp/service/BatimentController.java#L27 expects two arguments. Inside the method it checks for them, but i suppose since the spring upgrade the parameters are checked before by spring which returns the code 400. @pierrejego can you confirm that ?

since ms2-cadastrapp calls getBatimentsByParcelle API first, it knows the list of existing buildings, and it's supposed to loop over them to call getBatiments with their building id. I don't understand where that getBatiments call without dnubat comes from, but that's where the issue is.

MaelREBOUX commented 2 years ago

I take note on the new behaviour of the cadastrapp 2.0 backend, certainly due to the upgrade of spring framework. I agree with this analysis :

ms2-cadastrapp wasn't supposed to call getBatiments without dnubat

And the @landryb PR seems correct to me because it makes sense to fix this on the front end side.