Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove duplicated quote to remove underscore create by browser #152

Merged
merged 2 commits into from
Mar 23, 2022

Conversation

pierrejego
Copy link
Member

Link to georchestra/cadastrapp#615 and #143

Tested on gis.jdev.fr environnement

@MaelREBOUX
Copy link
Member

👍

Copy link
Collaborator

@tdipisa tdipisa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried to reproduce this on our DEV instance here where we are using the last version of the extensions (eg. cadastrapp in this case). As far as I can see, I'm not able to reproduce this issue.

Is the use case depicted in the GIF below correct @pierrejego? Otherwise can you kindly indicate it?

csv_file

I've tested the use case above on the following browsers (versions of browsers are the last ones available):

Chrome: 99.0.4844.74
FF: 98.0.1
Edge: 99.0.1150.46

However, if in some cases this issue exists, directly forcing a replacement I'm not sure is the right fix do. We should at least understand why these wrong characters are present in the file name instead.

I will ask developers (@dsuren1 and @offtherailz) to do a check on this as soon as the use case to reproduce is confirmed or clarified.

Which version of cadastrapp are you using? Which browser and versions of the browser are you using?

@catmorales
Copy link

catmorales commented Mar 21, 2022

For the "_" , use case is decribed in georchestra/cadastrapp#615.
However, i will update mapstore2-georchestra, mapstore2-cadastrapp and cadastrapp to test with the latest versions of both.

@tdipisa
Copy link
Collaborator

tdipisa commented Mar 21, 2022

@catmorales are you able to reproduce the same using the context in our DEV instance?

@catmorales
Copy link

catmorales commented Mar 21, 2022

@catmorales are you able to reproduce the same using the context in our DEV instance?

@tdipisa , No on your dev instance i don't reproduce it because i think that you are not on the last cadastrapp back end version. Cadastrapp BE has been updated for using best recent geotools version and other things. That's why we have these problems and why @pierrejego did this pull request.

I reproduce it on portail-test , you can try it in the "master-urba-foncier" context.
with the "_" on chrome, i have updated all this morning in portail-test

Cadastrapp (BE) --> Cadastrapp V2.0
Mapstore2-cadastrapp --> rc18
Mapstore2-georchestra --> 21.02.xx (image docker geosolutions)

@catmorales
Copy link

catmorales commented Mar 21, 2022

To clarify i will give you details and uses cases on:

@pierrejego
Copy link
Member Author

Hi @tdipisa ,
this problem came only with new backend. We have changed technologie from Jaxrs to SpringMVC. this made a change on headers. He have additionnal quote " in filename field.
This is ok with rfc http https://www.rfc-editor.org/rfc/rfc6266 but not managed with Chrome Engine which replace quote (and special chars) by underscore _
This PR is just made to remove those chars so we don't have any underscore.

We have discuss about it yesterday afternoon with Catherine, Those few change won't have any impact on current version. I therefore allow myself to merge the PR. Do not hesitate to contact me if needed

Regards

@pierrejego pierrejego merged commit d0ea5e9 into master Mar 23, 2022
@tdipisa
Copy link
Collaborator

tdipisa commented Mar 25, 2022

Hi @tdipisa , this problem came only with new backend. We have changed technologie from Jaxrs to SpringMVC. this made a change on headers. He have additionnal quote " in filename field. This is ok with rfc http https://www.rfc-editor.org/rfc/rfc6266 but not managed with Chrome Engine which replace quote (and special chars) by underscore _ This PR is just made to remove those chars so we don't have any underscore.

We have discuss about it yesterday afternoon with Catherine, Those few change won't have any impact on current version. I therefore allow myself to merge the PR. Do not hesitate to contact me if needed

Regards

Dear @pierrejego ok. In my opinion we should better keep track of this at least by putting a comment in the code for future reference including also the link to this PR and maybe also a link to a new issue aimed to investigate the root cause of the problem for a definitive fix.

@landryb
Copy link
Member

landryb commented Mar 25, 2022

In my opinion we should better keep track of this at least by putting a comment in the code for future reference including also the link to this PR and maybe also a link to a new issue aimed to investigate the root cause of the problem for a definitive fix.

from my understanding there's no other "definitive fix", the root cause is identified: underlying libraries changed, adding the quotes is the behaviour of the new library upon which we have no control, so things will be this way and have to be dealt with. The commit doesn't break compatibility with previous versions (filenames are generated by the backend and cant contain user-generate strings so no risk of having " or ' in there), so things are safe for users using the older backend (as you witnessed since you didnt manage to reproduce the issue) - if you update the plugin/client side on your install things will still work as before.

as for putting comments, i think everything is properly "documented" in commit messages which link back to this PR which has all the references needed for related issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants