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

apps/votes/models//migrations: change primary key of VotingToken to i… #3987

Merged
merged 1 commit into from
Nov 26, 2021

Conversation

Rineee
Copy link
Contributor

@Rineee Rineee commented Nov 25, 2021

…d and migrate foreign keys of TokenVotes

It was pretty annoying with the foreign keys, I tried a lot and in the end was mainly following https://blog.hexack.fr/en/change-the-primary-key-of-a-django-model.html

Hope this works! 😬

I think dev has an empty table there anyway, do we want to put sth in there to see if it works maybe? Or not risk it ..

Copy link
Contributor

@sabinammm sabinammm left a comment

Choose a reason for hiding this comment

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

After the migration, proposals show up twice:
image

@fuzzylogic2000
Copy link
Contributor

After the migration, proposals show up twice:

@sabinammm That was also already there before the migration. We do currently show the list twice. When checking a PR, pleqase have a look before and after you use the new code. That way, you can check if the PR really fixes what it promises and you will also notice what was introduced by the PR and what was there before.

@sabinammm
Copy link
Contributor

After the migration, proposals show up twice:

@sabinammm That was also already there before the migration. We do currently show the list twice. When checking a PR, pleqase have a look before and after you use the new code. That way, you can check if the PR really fixes what it promises and you will also notice what was introduced by the PR and what was there before.

Yes, what I did was that I had two databases: one with the migration one without it. Then I would switch using either one to compare, using one and the same list that I was looking at. But since I don't understand the steps of the migration, this is probably not helpful and related to the list anyway.

@Rineee Rineee force-pushed the ks-2021-11-change-primary-key-voting-token branch from 30fd715 to fa5d9f0 Compare November 26, 2021 11:05
@github-actions
Copy link

Coverage report

Total coverage

Status Category Percentage Covered / Total
🔴 Statements 3.06% 38/1241
🔴 Branches 2.8% 20/714
🔴 Functions 4.15% 17/410
🔴 Lines 6.76% 247/3653

Status of coverage: 🟢 - ok, 🟡 - slightly more than threshold, 🔴 - under the threshold

Report generated by 🧪jest coverage report action from fa5d9f0

@Rineee
Copy link
Contributor Author

Rineee commented Nov 26, 2021

I took out the part with adding the id and filling it manually (because actually Django does it when adding id as an AutoField) and changed the order of the primary key change (so now it first deletes it from token and then adds it for id) and it seems fine now 😀

@Rineee Rineee requested a review from sabinammm November 26, 2021 11:18
@fuzzylogic2000
Copy link
Contributor

After the migration, proposals show up twice:

@sabinammm That was also already there before the migration. We do currently show the list twice. When checking a PR, pleqase have a look before and after you use the new code. That way, you can check if the PR really fixes what it promises and you will also notice what was introduced by the PR and what was there before.

Yes, what I did was that I had two databases: one with the migration one without it. Then I would switch using either one to compare, using one and the same list that I was looking at. But since I don't understand the steps of the migration, this is probably not helpful and related to the list anyway.

@sabinammm Ah! The current main and this PR had different code in the template and it was already fixed on main. Didn't get that. I think now @Rineee has also rebased and the list should also be fine.

Copy link
Contributor

@fuzzylogic2000 fuzzylogic2000 left a comment

Choose a reason for hiding this comment

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

Ok, for me it looks like it switched the corresponding project for two Tokens? Which doesn't make sense... And I didn't check it thoroughly enough in the first round. And didn't save the DB. So, will try again. But here is my current trial:

>>> VotingToken.objects.all()
<QuerySet [<VotingToken: VotingToken object (Cju7EquhJO8Z8G5rUIPZ6Q)>, <VotingToken: VotingToken object (jyrKmdaJSZJtG2JQkVBopA)>, <VotingToken: VotingToken object (c6kwgqZUkcl7N86Ih3Vgtg)>]>
>>> prop=Proposal.objects.first()
>>> tv=TokenVote(content_object=prop, token=VotingToken.objects.first())
>>> tv.save()
>>> tv.token
<VotingToken: VotingToken object (Cju7EquhJO8Z8G5rUIPZ6Q)>
>>> tv=TokenVote(content_object=prop, token=VotingToken.objects.last())
>>> tv.save()
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/home/katharina/a4-meinberlin/meinberlin/apps/votes/models.py", line 111, in save
    self.full_clean()
  File "/home/katharina/a4-meinberlin/venv/lib/python3.8/site-packages/django/db/models/base.py", line 1238, in full_clean
    raise ValidationError(errors)
django.core.exceptions.ValidationError: {'token': ['This token is not valid. It might be expired or not valid for this project.']}
>>> VotingToken.objects.last().project
<Project: Bürgerhaushalt 2>

So, I had two tokens for "Bürgerhaushalt 3" and one for "Bürgerhaushalt 2". But I didn't check the token for the latter (from the last(), I would suspect, it would be <VotingToken: VotingToken object (c6kwgqZUkcl7N86Ih3Vgtg)>). But after the migration:

>>> VotingToken.objects.all()
<QuerySet [<VotingToken: VotingToken object (1)>, <VotingToken: VotingToken object (2)>, <VotingToken: VotingToken object (3)>]>
>>> VotingToken.objects.first().token
'Cju7EquhJO8Z8G5rUIPZ6Q'
>>> VotingToken.objects.first().project
<Project: Bürgerhaushalt 3>
>>> VotingToken.objects.get(pk=2).project
<Project: Bürgerhaushalt 2>
>>> VotingToken.objects.get(pk=2).token
'jyrKmdaJSZJtG2JQkVBopA'
>>> VotingToken.objects.last().project
<Project: Bürgerhaushalt 3>
>>> VotingToken.objects.last().token
'c6kwgqZUkcl7N86Ih3Vgtg'

So the order of the qs is the same, but the project is switched? And I cannot look into the first DB version again. 😠 Will just try again.

Copy link
Contributor

@fuzzylogic2000 fuzzylogic2000 left a comment

Choose a reason for hiding this comment

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

It is all fine!

>>> for obj in VotingToken.objects.all():
...     print(obj.token, ' - pro: ', obj.project, ' - mod: ', obj.module)
... 
L_cViRrrOeoOwKbhYw9JYQ  - pro:  Bürgerhaushalt 3  - mod:  Bürgerhaushalt 3 (1)
duKv8VDPC8rm7fJ159DQSA  - pro:  Bürgerhaushalt 2  - mod:  Bürgerhaushalt 2 (1)
mENw-_5720xIcQ7ezg3iIQ  - pro:  Bürgerhaushalt 3  - mod:  Bürgerhaushalt 3 (1)
IQhpCwnZe9AJDJeN0IC_0A  - pro:  proooojekt  - mod:  proooojekt (1)
HDhHo40wXe6XMCAKr2_bOg  - pro:  brainstorming lalala kdjhf dfaswhdef ahedfs hal  - mod:  brainstorming lalala kdjhf dfaswhdef ahedfs hal (2)
zULLowr-MwZEdZNqaBa1SQ  - pro:  Bürgerhaushalt 3  - mod:  Bürgerhaushalt 3 (1)

and after:

>>> for obj in VotingToken.objects.all():
...     print(obj.token, ' - pro: ', obj.project, ' - mod: ', obj.module)
... 
L_cViRrrOeoOwKbhYw9JYQ  - pro:  Bürgerhaushalt 3  - mod:  Bürgerhaushalt 3 (1)
duKv8VDPC8rm7fJ159DQSA  - pro:  Bürgerhaushalt 2  - mod:  Bürgerhaushalt 2 (1)
mENw-_5720xIcQ7ezg3iIQ  - pro:  Bürgerhaushalt 3  - mod:  Bürgerhaushalt 3 (1)
IQhpCwnZe9AJDJeN0IC_0A  - pro:  proooojekt  - mod:  proooojekt (1)
HDhHo40wXe6XMCAKr2_bOg  - pro:  brainstorming lalala kdjhf dfaswhdef ahedfs hal  - mod:  brainstorming lalala kdjhf dfaswhdef ahedfs hal (2)
zULLowr-MwZEdZNqaBa1SQ  - pro:  Bürgerhaushalt 3  - mod:  Bürgerhaushalt 3 (1)

But yes, let's add some VotingTokens and TokenVotes to dev before merging. Otherwise all the fun was only for the fun of it. :p

@Rineee
Copy link
Contributor Author

Rineee commented Nov 26, 2021

It is all fine!

Yay, cool!

But yes, let's add some VotingTokens and TokenVotes to dev before merging. Otherwise all the fun was only for the fun of it. :p

I added some tokens and votes, so lets seeee :)

@Rineee Rineee merged commit 33e9789 into main Nov 26, 2021
@Rineee Rineee deleted the ks-2021-11-change-primary-key-voting-token branch November 26, 2021 14:37
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.

3 participants