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

Enable basket export for kek #183

Merged
merged 36 commits into from
Dec 20, 2024
Merged

Enable basket export for kek #183

merged 36 commits into from
Dec 20, 2024

Conversation

domi4484
Copy link
Collaborator

Based on #182

@domi4484 domi4484 force-pushed the enableBasktetExportForKEK branch 2 times, most recently from c8ce769 to e2cef2a Compare November 20, 2024 21:12
@ponceta ponceta closed this Nov 21, 2024
@ponceta ponceta reopened this Nov 21, 2024
@ponceta ponceta self-requested a review November 21, 2024 10:13
@ponceta ponceta added enhancement New feature or request ili2pg issues that require upstream ili2pg changes fix INTERLIS labels Nov 21, 2024
@ponceta ponceta added this to the Datamodel 1.6.3 Plugin 1.6.4 milestone Nov 21, 2024
@domi4484 domi4484 force-pushed the enableBasktetExportForKEK branch from d5a2b35 to ea1a26f Compare November 21, 2024 10:14
@ponceta
Copy link
Member

ponceta commented Nov 21, 2024

#181 will be rebased after this.

@sjib
Copy link
Contributor

sjib commented Nov 26, 2024

======================================================================
ERROR: test_case_b_export_complete_qgep_to_xtf (qgepqwat2ili.tests.test_qgep.TestQGEPUseCases.test_case_b_export_complete_qgep_to_xtf)
# B. export the whole QGEP model to INTERLIS
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.12.7/x64/lib/python3.12/site-packages/sqlalchemy/engine/base.py", line 1256, in _execute_context
    self.dialect.do_executemany(
  File "/opt/hostedtoolcache/Python/3.12.7/x64/lib/python3.12/site-packages/sqlalchemy/dialects/postgresql/psycopg2.py", line 912, in do_executemany
    cursor.executemany(statement, parameters)
psycopg2.errors.NotNullViolation: null value in column "t_basket" violates not-null constraint
DETAIL:  Failing row contains (0, null, organisation, ch13p7mzOG000002).

@domi4484 Can you fix this also?

@sjib sjib changed the title Enable basktet export for kek Enable basket export for kek Dec 3, 2024
@sjib
Copy link
Contributor

sjib commented Dec 9, 2024

@domi4484 It looks like organisation also needs a t_basket column. Could you finish this asap - there are other developments waiting for this to be finished.

@domi4484
Copy link
Collaborator Author

domi4484 commented Dec 9, 2024

@sjib the basket column get created. But for some reason sometimes the value does not get populated.

@@ -165,7 +165,7 @@ def base_common(self, row, type_name):
"t_id": self.get_tid(row),
}

if self.current_basket is not None:
if self.current_basket is not None and type_name != "organisation":
Copy link
Contributor

@sjib sjib Dec 9, 2024

Choose a reason for hiding this comment

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

@domi4484 Here you are not writing the current_basket if you have class organisation - is this the problem? The tests fail on organisation data.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@domi4484
Copy link
Collaborator Author

On INTERLIS import 2015 this works already, see example above where based on the text values VSA and SBU two new organisations are created.

In the import I don't see any code handling those values. Where is this implemented?

This is handeled here: def create_or_update_organisation(name):

def create_or_update_organisation(name):

Ok but there is no special handling of the values "VSA" and "SBU" right? 2 "normals" organisation get created with the default prefix

@sjib
Copy link
Contributor

sjib commented Dec 13, 2024

On INTERLIS import 2015 this works already, see example above where based on the text values VSA and SBU two new organisations are created.

In the import I don't see any code handling those values. Where is this implemented?

This is handeled here: def create_or_update_organisation(name):

def create_or_update_organisation(name):

Ok but there is no special handling of the values "VSA" and "SBU" right? 2 "normals" organisation get created with the default prefix

Yes, we have nothing hardcoded. Why should we?
But we can handle whether an organisation comes with its identifier such as "VSA" and "SBU" or with its obj_id such as ch080qwzPR000012

@domi4484
Copy link
Collaborator Author

On INTERLIS import 2015 this works already, see example above where based on the text values VSA and SBU two new organisations are created.

In the import I don't see any code handling those values. Where is this implemented?

This is handeled here: def create_or_update_organisation(name):

def create_or_update_organisation(name):

Ok but there is no special handling of the values "VSA" and "SBU" right? 2 "normals" organisation get created with the default prefix

Yes, we have nothing hardcoded. Why should we? But we can handle whether an organisation comes with its identifier such as "VSA" and "SBU" or with its obj_id such as ch080qwzPR000012

I am just trying to understand why those ID are making problems in this PR that in fact doesn't change anything in the import. And I think I found out now:

The test test_case_f_export_selection_sia405 which is failing with the organisation ID error doesn't explicitly set up the database as other tests do.

  • This means it uses the setup of another test.
  • But test are not guaranteed to run in a special order -> this test is probably in this case not using the demodata as a base and for this reason VSA and SBU get created with an invalid ID

qgepqwat2ili/qgep/export.py Outdated Show resolved Hide resolved
qgepqwat2ili/qgepdss/export.py Outdated Show resolved Hide resolved
qgepqwat2ili/qgepsia405/export.py Outdated Show resolved Hide resolved
qgepqwat2ili/tests/test_qgep.py Outdated Show resolved Hide resolved
@sjib
Copy link
Contributor

sjib commented Dec 16, 2024

I tried to test the current status - No output in KEK file:

<?xml version="1.0" encoding="UTF-8"?><TRANSFER xmlns="http://www.interlis.ch/INTERLIS2.3">
<HEADERSECTION SENDER="ili2pg-5.2.0-375168b6abf225834531d7aa07a1fbb159937361" VERSION="2.3"><MODELS><MODEL NAME="VSA_KEK_2019_LV95" VERSION="20.01.2021" URI="http://www.vsa.ch/models"></MODEL></MODELS></HEADERSECTION>
<DATASECTION>
<VSA_KEK_2019_LV95.KEK BID="5">
</VSA_KEK_2019_LV95.KEK>
</DATASECTION>
</TRANSFER>

@sjib
Copy link
Contributor

sjib commented Dec 16, 2024

@domi4484 Here some testdata for importing VSA-KEK 2019 data:
testdata_vsa_kek_2019.zip

@domi4484
Copy link
Collaborator Author

I tried to test the current status - No output in KEK file:

Yes in the tests output it looks empty as well.

Thanks for the additional testdata. I think I found the issue. The objects are put into the wrong basket and so ili2db can't export them. Ill try to fix that

@domi4484
Copy link
Collaborator Author

Now the .xtf file is filled with data for me locally. On the pipeline the file is still empty

@ponceta ponceta closed this Dec 17, 2024
@ponceta ponceta reopened this Dec 17, 2024
@sjib
Copy link
Contributor

sjib commented Dec 19, 2024

@domi4484 Can you finish this today so we can make the release of 1.6.6 tomorrow?

@ponceta ponceta marked this pull request as ready for review December 20, 2024 09:07
@ponceta ponceta merged commit bce150e into master Dec 20, 2024
3 of 5 checks passed
@ponceta ponceta deleted the enableBasktetExportForKEK branch December 20, 2024 09:08
@ponceta
Copy link
Member

ponceta commented Dec 20, 2024

As this is working on local installations, we will merge it, test it and fix the CI in 2025.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fix ili2pg issues that require upstream ili2pg changes INTERLIS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants