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

fix: migrate 'None' Strings to NULL in Changes Table #2620

Merged
merged 6 commits into from
Feb 9, 2025

Conversation

anj20
Copy link
Contributor

@anj20 anj20 commented Feb 4, 2025

Purpose of PR?: This PR fixes incorrect storage of "None" as a string and ensures it is properly migrated to NULL.

Fixes #2590

Does this PR introduce a breaking change? No, this PR does not introduce any breaking changes. It only updates stored data to ensure consistency.

Checklist:

Copy link
Member

@ReimarBauer ReimarBauer left a comment

Choose a reason for hiding this comment

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

please read in develop the documentation https://github.com/Open-MSS/MSS/blob/develop/docs/development.rst#changing-the-database-model

and incorporate the change into the existing migration script.

@anj20
Copy link
Contributor Author

anj20 commented Feb 4, 2025

please read in develop the documentation https://github.com/Open-MSS/MSS/blob/develop/docs/development.rst#changing-the-database-model

and incorporate the change into the existing migration script.

done. can u plz review it?

@anj20 anj20 requested a review from ReimarBauer February 4, 2025 20:08
@ReimarBauer
Copy link
Member

@anj20 look on the tests

@anj20
Copy link
Contributor Author

anj20 commented Feb 4, 2025

@anj20 look on the tests

updated the function to pass the case for sqlite

@ReimarBauer
Copy link
Member

Copy link
Member

@ReimarBauer ReimarBauer left a comment

Choose a reason for hiding this comment

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

@anj20 anj20 requested a review from ReimarBauer February 7, 2025 13:51
@@ -21,13 +21,17 @@ def upgrade():
# ### commands auto generated by Alembic - please adjust! ###
with op.batch_alter_table('users', schema=None) as batch_op:
batch_op.add_column(sa.Column('profile_image_path', sa.String(length=255), nullable=True))

with op.batch_alter_table('changes', schema=None) as batch_op:
Copy link
Member

Choose a reason for hiding this comment

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

I have in the past stored the version_name and the comment default as string 'None''. And now I want to convert these values to None.

I think the solution is to just do that in upgrade, op.execute(
"UPDATE changes SET version_name = NULL WHERE version_name = 'None';"
)

and in the downgrade just the opposite

op.execute(
"UPDATE changes SET version_name = 'None' WHERE version_name IS NULL;"
)

feel free to add the other one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

still it is failing some test case

Copy link
Member

Choose a reason for hiding this comment

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

two because of timeout and the third is independent to your change

FAILED tests/_test_msui/test_sideview.py::Test_SideViewWMS::test_server_getmap - pytestqt.exceptions.TimeoutError: Signal canceled() not emitted after 5000 ms
FAILED tests/_test_msui/test_topview.py::Test_TopViewWMS::test_server_getmap - pytestqt.exceptions.TimeoutError: Signal canceled() not emitted after 5000 ms
FAILED tests/_test_mswms/test_mss_plot_driver.py::Test_VSec::test_VS_HorizontalVelocityStyle_01 - Failed: CALL ERROR: Exceptions caught in Qt event loop:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @ReimarBauer ,
i am not getting the reason why this test case is getting time out. I think it's not related to the changes i made.

Copy link
Member

Choose a reason for hiding this comment

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

Hi @anj20 the timeouts I started to investigate at
#2626

The changes describe what I suggested. That is all fine. I'll merge after I tried it with a real world example likly at the weekend. But because of the weekend I have not soo much time for that.

Copy link
Member

@ReimarBauer ReimarBauer left a comment

Choose a reason for hiding this comment

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

Thx

@ReimarBauer ReimarBauer merged commit 8811fc3 into Open-MSS:develop Feb 9, 2025
9 of 11 checks passed
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.

add a migration step for wrong used None str
2 participants