-
Notifications
You must be signed in to change notification settings - Fork 6
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
Jd 2023 02 codes delete background task #4965
Conversation
goapunk
commented
Feb 15, 2023
- delete codes in background tasks
- fix some tests
- update translations
681184b
to
c0dec1b
Compare
maybe fixes #4963 |
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.
Lgtm! Although locally it does take the same amount of time for me (sth like 30 sec), but I guess worth trying on dev.
Do you also want to have a look @fuzzylogic2000 ?
Maybe we first deploy this branch and test it? |
Good idea, yes! |
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.
My thought when setting out to do this the first time was to have an extra package model instead of the number. That way when the package is downloaded, we only need to update that in the package model and can delete the plain tokens whenever it happens.
But let's try if this works first! When the actual downloading is the problem, that also won't help.
c0dec1b
to
7b3a747
Compare
6548163
to
42d0b97
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.
Is this ready to be reviewed?
And do we still need the package number here or is a package enough?
6578c19
to
930b1e1
Compare
42d0b97
to
4d447a8
Compare
This is ready now I think. I'm not sure what you mean with the package_numbers? |
4d447a8
to
54576e1
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.
The code looks good, but I haven't tried it out yet.
I still don't get why we need the package number and can not just use the package without the number. But I will have another look tomorrow morning with a fresh brain.
Maybe @Rineee can also check it out?
54576e1
to
eedb664
Compare
72dca5c
to
220c051
Compare
@Rineee @fuzzylogic2000 this is ready for another round of reviews. But don't merge yet, needs squash and translations |
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.
Yeah! It reads well. And it nearly works. But when I make a big batch, the "created" is False and not shown in the download section.
>>> TokenPackage.objects.all()
<QuerySet [<TokenPackage: TokenPackage object (1)>, <TokenPackage: TokenPackage object (2)>, <TokenPackage: TokenPackage object (3)>]>
>>> TokenPackage.objects.all()[0].module
<Module: Bürger*innenhaushalt (3 Phasen) - new bürherhaushalt s (1)>
>>> TokenPackage.objects.all()[1].module
<Module: Bürger*innenhaushalt (3 Phasen) - new bürherhaushalt s (1)>
>>> TokenPackage.objects.all()[2].module
<Module: Bürger*innenhaushalt (3 Phasen) - new bürherhaushalt s (1)>
>>> TokenPackage.objects.all()[0].size
10
>>> TokenPackage.objects.all()[1].size
1000000
>>> TokenPackage.objects.all()[2].size
200
>>> TokenPackage.objects.all()[0].created()
True
>>> TokenPackage.objects.all()[1].created()
False
>>> TokenPackage.objects.all()[2].created()
False
Ah! I didn't wait long enough and forgot about the batch size of 100.000. So, it was simply that not all tasks were run yet. 🤦 It works perfectly fine! |
220c051
to
8057ccb
Compare
@fuzzylogic2000 @Rineee rebased and added the translations, is ready to be merged now |
meinberlin/apps/votes/templates/meinberlin_votes/token_export_dashboard.html
Outdated
Show resolved
Hide resolved
8057ccb
to
b0a9f1b
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.
The admin part does not really work, it throws an error when creating a token and also when changing a token, it adds 1 to the package size..
Also it did not show any packages in the dashboard, but that is most likely a me problem, I will check again tomorrow with fresh brain!
b0a9f1b
to
f38128b
Compare
I managed again to produce an error 🙈 I had a package in which all codes were inactive and downloading that throws an error.. I dont know if that is too much of an edge case, but probably we should fix that anyway? Or what do you think @fuzzylogic2000 ? Apart from that it works perfectly and from my side good to merge! |
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.
Nice! I think it is a good solution with the packages and the size and so on!
Maybe we can also add the packages to the django-admin, so that they can be deleted if need be. But not in this PR anymore! :)
@Rineee Yeeees, I didn't try that, but will also do that. We should probably fix that even though it is an edge case. |
@goapunk actually thought of it and raises the bad request here: https://github.com/liqd/a4-meinberlin/pull/4965/files#diff-ef2af1f23503a2f006e8048ad40751390211bb74ca18c6882369e290b66f022bR67 |
@goapunk @Rineee I agree. Let's fix that by not showing these packages. I will merge anyway and hope that noone tests that. :p
|