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

14 add additional date fields for lifecycle status events #249

Merged

Conversation

msorvoja
Copy link
Contributor

@msorvoja msorvoja commented Mar 14, 2024

Fix #14

@Rikuoja @lehtojaa I couldn't find English translations for all lifecycle events, so please let me know if you disagree with any of them.

@msorvoja msorvoja requested a review from Rikuoja March 14, 2024 07:15
@msorvoja msorvoja linked an issue Mar 14, 2024 that may be closed by this pull request
@msorvoja msorvoja requested a review from lehtojaa March 14, 2024 07:15
@Rikuoja
Copy link
Contributor

Rikuoja commented Mar 14, 2024

Fix #14

@Rikuoja @lehtojaa I couldn't find English translations for all lifecycle events, so please let me know if you disagree with any of them.

@msorvoja They are in English here, so you can use these terms https://koodistot.suomi.fi/codescheme;registryCode=rytj;schemeCode=kaavaelinkaari

Copy link
Contributor

@Rikuoja Rikuoja left a comment

Choose a reason for hiding this comment

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

Looks good, but as per usual, let's do it another way after all!

Reworded the issue so that we should add a completely new table instead!

Copy link
Contributor

@Rikuoja Rikuoja left a comment

Choose a reason for hiding this comment

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

Looks very good!

Perhaps we could also add, for completeness' sake, a test fixture for the new model in conftest.py, and a test function for all the relationships in test_models.py. Just rebase this on main to get all the other pytest fixtures in conftest.py and test_models.py so you can add your own.

For ease of testing, let's just test (for now) a lifecycle date that may refer to plan, plan regulation and plan proposition simultaneously.

Of course, the right way would be to test three lifecycle statuses that each only refer to one of the three tables, since that is the way they will be used in practice, but this will do. Later, if we need to add a constraint function that requires that only one of the three fields is filled, then this test must be rewritten.

So this would mean a function like

def test_lifecycle_date(
    session: Session,
    lifecycle_date_instance: models.LifeCycleDate,
    code_instance: codes.LifeCycleStatus,
    plan_instance: models.Plan,
    plan_regulation_instance: models.PlanRegulation,
    plan_proposition_instance: models.PlanProposition,
):
    # non-nullable lifecycle date relations
    assert (
        lifecycle_date_instance.lifecycle_status is code_instance
    )
    assert code_instance.lifecycle_dates == [
        lifecycle_date_instance
    ]
    # nullable lifecycle date relations
    assert (
        lifecycle_date_instance.plan is None
    )
    assert plan_instance.lifecycle_dates == []
    lifecycle_date_instance.plan = plan_instance
    assert (
        lifecycle_date_instance.plan_regulation is None
    )
    assert plan_regulation_instance.lifecycle_dates == []
    lifecycle_date_instance.plan_regulation = plan_regulation_instance
    assert (
        lifecycle_date_instance.plan_proposition is None
    )
    assert plan_proposition_instance.lifecycle_dates == []
    lifecycle_date_instance.plan_proposition = plan_proposition_instance
    session.flush()
    ...

and then after session.flush(), check that the lifecycle_date_instance has all three relations set, and all the other three instances have this lifecycle_date_instance in their lifecycle_dates list.

database/models.py Outdated Show resolved Hide resolved
database/models.py Outdated Show resolved Hide resolved
database/models.py Outdated Show resolved Hide resolved
database/models.py Show resolved Hide resolved
@msorvoja
Copy link
Contributor Author

@Rikuoja I added the test function, test fixture and other changes you requested. Should be good to merge now!

Copy link
Contributor

@Rikuoja Rikuoja left a comment

Choose a reason for hiding this comment

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

Looks very good!

@Rikuoja Rikuoja merged commit e0ffcf4 into main Mar 28, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Add additional date fields for lifecycle status events
3 participants