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

GSPOE-132 add index column for disclaimer and general_information #1753

Merged
merged 4 commits into from
Nov 28, 2023

Conversation

mki-c2c
Copy link
Contributor

@mki-c2c mki-c2c commented May 16, 2023

Fixes #1706

harmonize database definitions according to #1387

@mki-c2c mki-c2c requested a review from michmuel May 16, 2023 13:45
@mki-c2c
Copy link
Contributor Author

mki-c2c commented May 16, 2023

I have implemented the additional "extract_index" columns. I have not enforced the not null constraint on the column, so it should be backward compatible. Sorting is done on this data when available.hello

@michmuel : as discussed (quite a while ago) could you please check with your data if the import will still work without errors ?

Copy link
Collaborator

@michmuel michmuel left a comment

Choose a reason for hiding this comment

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

Looks good to me. I have only one question concerning the different implementations of "extract_index" in the two db models.

Merging this PR would require that users have to have a column "extract_index" in the db tables "disclaimer" and "general_information". However, values could be Null. Right?

In general, I do not understand the reason for the extract_index for disclaimers and general information as it is - as far as I see - not part of the extract schema and the placing of these texts in the pdf is clearly defined.

pyramid_oereb/contrib/data_sources/standard/models/main.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 12, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4a920ad) 77.68% compared to head (55fb095) 77.71%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1753      +/-   ##
==========================================
+ Coverage   77.68%   77.71%   +0.02%     
==========================================
  Files         127      127              
  Lines        5271     5277       +6     
==========================================
+ Hits         4095     4101       +6     
  Misses       1176     1176              
Flag Coverage Δ
unittests 77.71% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mki-c2c
Copy link
Contributor Author

mki-c2c commented Sep 12, 2023

Hello @michmuel I am sorry, it took me a long while to finalize this topic.
Yes you are right, the extract_index for disclaimer and general information is optional and it is ignored if not provided.

The field is just added for consistency with the definitions in http://models.geo.admin.ch/V_D/OeREB/OeREBKRMkvs_V2_0.ili
If the index is provided, the data can be imported without loss of information.

If other data sources do not use the index, there is no difference.

Furthermore, I added some tests, to make sure the sorting is executed correctly.

dev/database/fed/disclaimer.json.xsl Outdated Show resolved Hide resolved
dev/database/fed/general_information.json.xsl Outdated Show resolved Hide resolved
pyramid_oereb/contrib/data_sources/standard/models/main.py Outdated Show resolved Hide resolved
pyramid_oereb/contrib/data_sources/standard/models/main.py Outdated Show resolved Hide resolved
@michmuel michmuel force-pushed the GSPOE-132_extract_index branch from 85d3982 to 0c52a52 Compare November 27, 2023 11:06
@michmuel michmuel requested review from michmuel and peterschaer and removed request for michmuel November 27, 2023 14:04
@michmuel michmuel requested review from voisardf and svamaa November 27, 2023 14:16
Copy link
Collaborator

@svamaa svamaa left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for finishing it ;)

@svamaa svamaa merged commit c12b572 into master Nov 28, 2023
12 checks passed
@svamaa svamaa deleted the GSPOE-132_extract_index branch November 28, 2023 07:08
@callsumzg
Copy link

Hi, thanks for updating this.

May I ask you why you implement breaking changes in a minor update?

Are you aware that this can cause problems when updating pyramid oereb when one is not aware of this?

We would really, really, from the bottom of our hearts, appreciate it, if you could please at least leave a note in the release notes. For example something like this:

Version 2.4.7

  • BREAKING CHANGE: Add extract_index to disclaimer and general infomation
  • Interlis bug fix
  • Library upgrades (geoalchemy2, SQLAlchemy, jsonschema, lxml, responses, urllib3, pypdf)
  • Test coverage improvements
  • Python 3.8 is no longer explicitly supported
  • Remove print proxy xml2pdf, no longer used by the community

Regards, and thank you for considering this

Christian Sieber

@michmuel
Copy link
Collaborator

@callsumzg Thanks for your note. You are right. Version 2.4.7 introduces changes to the db structure of the standard model (one additional field "extract_index" for tables "disclaimer" and "general_information" in the main schema). That is not obvious for a user by means of the change log and can require additional time for the implementation of configuration changes and/or the error search. Sorry for the inconvenience. We will try to avoid similar things.

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.

Provide new index attributes
4 participants