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

Create vw_tww_additional_wastewater_structure. #42

Merged
merged 58 commits into from
Jul 15, 2024

Conversation

cymed
Copy link
Contributor

@cymed cymed commented Dec 5, 2023

Tackles #38
Additional ws types for drainless_toilet, wwtp_structure and small_treatment_plant are handled separately to not blow up the main ws view. Also, no extra-cols are allowed.

@ponceta
Copy link
Member

ponceta commented Dec 6, 2023

CI question :
Error: Process completed with exit code 137.
@3nids are there no failure logs in that case?

@3nids
Copy link
Contributor

3nids commented Dec 6, 2023

@ponceta I added the docker logs

to is a reserved keyword, I'd suggest using dt instead

@ponceta
Copy link
Member

ponceta commented Dec 7, 2023

Thank you @3nids

This would be vw_tww_additional_wastewater_structure instead of vw_additional_wastewater_structure

(superclass views (superclass views that integrate all subclass attributes with prefixes to the attributes of the subclass) should be named vw_txx_superclass (example: vw_tww_maintenance_event). -> dev_guide_views)

@ponceta ponceta added enhancement New feature or request datamodel Concerns the datamodel labels Dec 7, 2023
@cymed
Copy link
Contributor Author

cymed commented Dec 7, 2023

I chose to because TO is the prefix of the OID-Postfix (ch000000TO000001)

@ponceta
Copy link
Member

ponceta commented Dec 7, 2023

@cymed you can also use toi to avoid the postgreSQL limitation

@cymed
Copy link
Contributor Author

cymed commented Dec 7, 2023

@sjib can we change the shortcut_en of drainless_toilet from TO to DT? DT is not used and was not used in QGEP,
Also I suggest keeping a list of all shortcuts of former tables somewhere to avoid problems with oid uniqueness.

@3nids
Copy link
Contributor

3nids commented Dec 7, 2023

@cymed no need to change the shortcut in the OID, it's only the keyword in the SQL code of the view (you use to as shortname for the table)

@cymed
Copy link
Contributor Author

cymed commented Dec 7, 2023

@cymed no need to change the shortcut in the OID, it's only the keyword in the SQL code of the view (you use to as shortname for the table)

It is more user friendly to use the OID shortcut as alias

cymed and others added 4 commits December 7, 2023 09:02
This push
- Alters the view name from vw_additional_wastewater_structure to vw_tww_additional_ws
- renames the drainless_toilet alias
- alters the import view to not depend on vw_tww_wastewater_structure during creation (allows re-calling create_views.py without dependency on existing views)
@ponceta
Copy link
Member

ponceta commented Dec 7, 2023

On the datamodel side it seems now working but we have still to adapt something because they will appear as unkown ws_types in the current wastewater_structure layer and I think it is not expected to work like in that way (opposite is also true).

WHERE ch.obj_id IS NULL;

image

@cymed
Copy link
Contributor Author

cymed commented Dec 7, 2023

On the datamodel side it seems now working but we have still to adapt something because they will appear as unkown ws_types in the current wastewater_structure layer and I think it is not expected to work like in that way (opposite is also true).

WHERE ch.obj_id IS NULL;

image

As my other PR #36 also touches vw_tww_wastewater_structure I suggest I finish #36 first to not run into compatibility issues

Edit: #36 needs adapation on pirogue if the select is to be done dynamically, prioritising this PR

@sjib
Copy link
Contributor

sjib commented Dec 7, 2023

@sjib can we change the shortcut_en of drainless_toilet from TO to DT? DT is not used and was not used in QGEP, Also I suggest keeping a list of all shortcuts of former tables somewhere to avoid problems with oid uniqueness.

I will change from TO to DT in English, French and Italien and to TA in German as 'AT' is also a SQL Keyword

@sjib
Copy link
Contributor

sjib commented Dec 7, 2023

Are there other SQL keyword that we should avoid?

  • 'AS'
  • 'AT'
  • 'TO'
  • ???

@cymed cymed added the help wanted Extra attention is needed label Apr 18, 2024
@cymed
Copy link
Contributor Author

cymed commented Apr 18, 2024

even though that PR has the same % handling as vw_tww_wastewater_structure, this one throws an error

@3nids
Copy link
Contributor

3nids commented Apr 18, 2024

you need to switch the double %% to simple % (next to a pirogue update)

@cymed
Copy link
Contributor Author

cymed commented Apr 18, 2024

you need to switch the double %% to simple % (next to a pirogue update)

then why did da34826 not work?

@3nids
Copy link
Contributor

3nids commented Apr 18, 2024

because you have other remaining double %%

@cymed cymed removed the help wanted Extra attention is needed label Apr 23, 2024
@cymed cymed marked this pull request as ready for review April 23, 2024 09:39
@cymed cymed added this to the TEKSI Wastewater 2024.1 milestone Apr 23, 2024
@cymed
Copy link
Contributor Author

cymed commented Apr 23, 2024

@ponceta ready for review

Copy link
Member

@ponceta ponceta left a comment

Choose a reason for hiding this comment

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

Let's go with this as it will soon be an issue for anyone wanting to use the new classes in TWW.

This could lead to a .qgs project adaptation and a documentation specific for these new entities.

@ponceta ponceta merged commit 11675f7 into teksi:main Jul 15, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datamodel Concerns the datamodel enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants