Skip to content

RIS light initial commit #1843

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

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

Conversation

Tschuppi81
Copy link
Contributor

@Tschuppi81 Tschuppi81 commented Jun 10, 2025

RIS: Models for RIS light

TYPE: Feature
LINK: ogc-2245

The drawing shows the new models that are not adopted from PAS to get a quick overview.

ris-light-class-diagram drawio

Copy link

linear bot commented Jun 10, 2025

Copy link

codecov bot commented Jun 10, 2025

Codecov Report

Attention: Patch coverage is 80.93385% with 147 lines in your changes missing coverage. Please review.

Project coverage is 87.17%. Comparing base (6948bb8) to head (d09ec75).
Report is 13 commits behind head on master.

Files with missing lines Patch % Lines
src/onegov/parliament/models/change.py 46.91% 43 Missing ⚠️
src/onegov/parliament/models/parliamentarian.py 78.15% 26 Missing ⚠️
src/onegov/parliament/models/attendence.py 66.66% 16 Missing ⚠️
src/onegov/pas/data_import.py 0.00% 12 Missing ⚠️
src/onegov/pas/excel_import_cli.py 0.00% 12 Missing ⚠️
src/onegov/parliament/models/commission.py 90.00% 5 Missing ⚠️
src/onegov/pas/views/settlement_run.py 61.53% 5 Missing ⚠️
...c/onegov/parliament/models/parliamentarian_role.py 91.83% 4 Missing ⚠️
src/onegov/pas/utils.py 55.55% 4 Missing ⚠️
src/onegov/pas/views/attendence.py 33.33% 4 Missing ⚠️
... and 8 more
Additional details and impacted files
Files with missing lines Coverage Δ
src/onegov/parliament/__init__.py 100.00% <100.00%> (ø)
src/onegov/parliament/i18n.py 100.00% <100.00%> (ø)
src/onegov/parliament/models/__init__.py 100.00% <100.00%> (ø)
src/onegov/parliament/models/party.py 100.00% <100.00%> (ø)
src/onegov/pas/collections/attendence.py 95.23% <100.00%> (+0.11%) ⬆️
src/onegov/pas/collections/change.py 100.00% <100.00%> (ø)
src/onegov/pas/collections/commission.py 100.00% <100.00%> (ø)
...rc/onegov/pas/collections/commission_membership.py 100.00% <100.00%> (ø)
src/onegov/pas/collections/legislative_period.py 100.00% <100.00%> (ø)
src/onegov/pas/collections/parliamentarian.py 100.00% <100.00%> (ø)
... and 48 more

... and 15 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6948bb8...d09ec75. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@Daverball Daverball left a comment

Choose a reason for hiding this comment

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

Since these models look like more or less 1:1 copies of the PAS models I think we should move the lowest common denominator into a parliament namespace with 'generic'``type, then PAS and RIS/Org (not sure yet if we need a RIS namespace) can each make their own subclasses with their own extensions.

We can also drop the pas_ prefix from the tables, just make sure to add a migration to rename the tables (it's possible SQLAlchemy will already have created an empty table for the new name, so you may need to drop it first, if it exists but is empty).

Otherwise this looks mostly good to me, although you're missing a ContentMixin for Commission (we may want a ContentMixin for all or at least most of the shared models, so we can more easily extend the specialized subclasses).

@Tschuppi81
Copy link
Contributor Author

I created the namespace parliament, as a tablename prefix I use par

@Daverball
Copy link
Member

We usually don't use table prefixes, but I don't have strong feelings about using one, so we can keep it.

I would keep the PAS specifc models like RateSet and SettlementRun in PAS, also for the polymorphic models I would put a subclass in the PAS namespace, even if that subclass doesn't add anything new yet, that way the PAS models can be extended, without needing to change the polymorphic identity in an upgrade step later on. (They already had a polymorphic identity other than 'generic' so you would need an upgrade step to fix that right now, only to potentially change it back later)

@Daverball
Copy link
Member

Make sure to add onegov.parlament to setup.cfg. I would probably also add an empty upgrade.py to the module and add it to the onegov_upgrades entry point in setup.cfg.

You could put the migration for renaming the tables in there, instead of putting it into PAS.

@Tschuppi81
Copy link
Contributor Author

Make sure to add onegov.parlament to setup.cfg. I would probably also add an empty upgrade.py to the module and add it to the onegov_upgrades entry point in setup.cfg.

You could put the migration for renaming the tables in there, instead of putting it into PAS.

Yes, that is what I missed!

Co-authored-by: David Salvisberg <[email protected]>
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.

2 participants