-
Notifications
You must be signed in to change notification settings - Fork 9
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
Tasotarkistus data to rent model #773
Tasotarkistus data to rent model #773
Conversation
I have questions about the model changes and what is needed for the initial UI changes, and what is needed in the future. Will discuss in a meeting with you. |
We talked with Jukka. There will likely be more changes coming to this PR when the UI implementation becomes more refined, especially the saving functionality. Then it becomes more apparent which properties need to be explicitly saved in database, and which ones can be derived from existing properties. For example, tasotarkistus type (20/20 or 20/10) must be saved along with the tasotarkistus, but maybe calculation dates can be derived from that type and the rent start date (e.g. 20 years from rent start date, then +10, then +10...). There is a feature request for notifications for upcoming tasotarkistus dates for the rents that have it in near future, maybe 3 years/2 years/1 year in advance. That notification implementation might be slow to execute without dedicated columns for tasotarkistus dates, where DB indexes could be created. But the first notification would happen sometime in the decade 2040, so this is probably just a suggestion in code rather than the final implementation. |
3359d3e
to
c13af49
Compare
4467e63
to
4504ef8
Compare
max_digits=8, | ||
null=True, | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I think we need a 3rd new property: type of the tasotarkistus (20v/20v) or (20v/10v).
The Rent class is getting slightly populated, but it might still be OK because I just removed 4 fields about the seasonal rent.
Another possibility is to create a new model for PeriodicRentAdjustment
which holds these 3 fields (index, adjustment type, and starting point figure). Then Rent would have a single ForeignKey property like periodic_rent_adjustment
that can be null if the rent doesn't use the feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you @henrinie-nc think?
c77da90
to
c06e0c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the discussed topics have been addressed, good work!
One topic stands, see the open comment about a adding a third new property to Rent class: type of the tasotarkistus (20v/20v) or (20v/10v).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This current state works for viewing. Editing view changes will come in a later PR
Add
old_dwellings_in_housing_companies_price_index
to rent model, to use tasotarkistus data on the Vuokrat tab.