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

Set audit sql #249

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Set audit sql #249

wants to merge 10 commits into from

Conversation

ponceta
Copy link
Member

@ponceta ponceta commented May 27, 2024

Set audit sql for history viewer

Fix #246

Set audit sql for history viewer
@ponceta ponceta added datamodel Concerns the datamodel fix Fixing something not working labels May 27, 2024
@ponceta ponceta self-assigned this May 27, 2024
@ponceta ponceta requested a review from cymed May 27, 2024 14:32
@ponceta ponceta added this to the TEKSI Wastewater 2024.0 milestone May 27, 2024
@cymed
Copy link
Contributor

cymed commented May 29, 2024

The example with tww_app poses a problem for CI

@ponceta
Copy link
Member Author

ponceta commented May 29, 2024

The example with tww_app poses a problem for CI

My main concern is :

Is this to be included in the changelog or in the application ? Especially because we will drop the audit triggers before any application update and recreate these afterwards.

I should maybe separate the logging table creation (which has to be persistent) with the audit trigger functions (which could evolve with time but sometimes without having a base datamodel change).

ponceta and others added 6 commits May 29, 2024 10:59
@cymed
Copy link
Contributor

cymed commented May 29, 2024

What if we add a metadata table somewhere that we can use to re-create the triggers on the app schema?

@ponceta
Copy link
Member Author

ponceta commented May 30, 2024

@cymed @3nids I suggest we continue discussion here :

I'm not sure I make myself clear.
I approved the PR before the addition of the hstore extension, added after and merged.
I think it is a good practice to prefer dedicated PRs for each change.
We had former discussions with Arnaud where I think we decided to have this audit functionality apart from the definition of the model. To my opinion, this should be at least optional.

  • I can reset the hstore extension status and integrate in this PR too keep track of why we have to have hstore as extension

  • As discussed, this functionnality is optional for the user / BUT the implementation stays as it is in QWAT and QGEP. (otherwise I won't be able to push a Release)
    Meaning

  • if the user wants to activate the logging, he has to set :

SELECT tww_sys.audit_view('tww_app.vw_tww_wastewater_structure', 'true'::BOOLEAN, 'field_to_ignore'::text[], 'obj_id'::text[])

https://github.com/teksi/wastewater/pull/249/files#diff-a0a816e4d5f6f64e6bcedba2a5c4f030afb95e787cb7046293737931c1d96387R286

As documented in https://teksi.github.io/wastewater/fr/user-guide/history/history.html#database-configuration.

@ponceta
Copy link
Member Author

ponceta commented May 31, 2024

image

@3nids
Copy link
Contributor

3nids commented Jun 4, 2024

I'm -1 on integrating such an old piece of code in a brand new solution as is. I think it would deserve first a point of situation on other solutions to avoid going with custom code here.

If you still want to move forward with, I think the trigger should lie in app at least.

-----------------------------------------------
-----------------------------------------------

CREATE OR REPLACE FUNCTION tww_sys.if_modified_func() RETURNS TRIGGER AS $body$
Copy link
Contributor

Choose a reason for hiding this comment

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

you should not create stuff in tww_sys from app!

Copy link
Contributor

Choose a reason for hiding this comment

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

Better move audit function into changelogs

@cymed
Copy link
Contributor

cymed commented Jun 4, 2024

I'm -1 on integrating such an old piece of code in a brand new solution as is. I think it would deserve first a point of situation on other solutions to avoid going with custom code here.

If you still want to move forward with, I think the trigger should lie in app at least.

For many end users, the functionality is necessary in production and we can't simply drop functionalities without notice. It is up to the technical group to decide if the functionality needs an overhaul or not. I suggest migrating it as it was and if needed overhaul it in a second step.

@ponceta
Copy link
Member Author

ponceta commented Jun 4, 2024

I agree with both of you :

  • We need such a functionnality
  • It would be much easier to level it to a new standard, this will not be trivial to upgrade afterwards

@3nids I check with you how and what we could support in this new module architecture.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datamodel Concerns the datamodel fix Fixing something not working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing auditing functionality 01_audit.sql
3 participants