[#4267] Activity data for old grants

[e18r]

Fixes #4267

A data migration is introduced, which generates grant-related activity data
that happened before grant-related activity was recorded.

The date before which grant-related activity will be generated was set to 2019-04-24 11:53:00 UTC, which is when PR #3859 was merged.

[codecov]

Codecov Report

Merging #4317 into master will increase coverage by 0.29%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4317      +/-   ##
==========================================
+ Coverage   30.04%   30.34%   +0.29%     
==========================================
  Files         213      213              
  Lines       17112    17113       +1     
  Branches     2311     2312       +1     
==========================================
+ Hits         5142     5193      +51     
+ Misses      11770    11713      -57     
- Partials      200      207       +7

Impacted Files	Coverage Δ
app/economy/models.py	58.41% <100%> (+0.41%)	⬆️
app/retail/emails.py	22.37% <0%> (+0.84%)	⬆️
app/marketing/mails.py	12.84% <0%> (+1.32%)	⬆️
app/dashboard/embed.py	31.6% <0%> (+3.44%)	⬆️
app/grants/models.py	58.55% <0%> (+8.88%)	⬆️
app/grants/templatetags/grants_extra.py	100% <0%> (+28.57%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73b868c...9a696dd. Read the comment docs.

[e18r]

@owocki @thelostone-mc @danlipert please review my work, thanks

[owocki]

migration LGTM . thanks for doing this @e18r !

[e18r]

Hi @danlipert, please don't forget to check this out. Also, if you need anything, don't hesitate. Thank you so much!

[danlipert]

Hey @e18r - this looks nice! Two questions: 1) were you able to test this effectively on your local? If not, a unit test would be nice. 2) Regarding the "deployment" date - since we don't have continuous deployment set up, that date is not accurate. I can probably figure out when the code was deployed exactly, but I'm wondering if you could make this code a bit more resilient such that it wouldn't create duplicate entries even without filtering the objects by the deployment date.

[e18r]

Hi @danlipert, 1) I was able to test it effectively (although with little data) on my local environment, but if you prefer, I can do the unit test as well. 2) I will make the migration more resilient by preventing duplicates. This can also help in case the migration is mistakenly run twice.

[e18r]

@danlipert I created a way to check if the activity exists before adding it. Check it out in my latest commit. For unique activity types (create grant and delete grant) it checks that there aren't other activities with that type for the current grant. For other activity types, it also checks that they were made by the same person, and at roughly the same time. The deployment date check is still there as an extra safety layer.

Regarding the unit test, have you guys established a way of unit testing data migrations in Gitcoin? I did some research, and found this SO question that suggests a way of doing it. However, maybe the simplest way to test it would be to download a subset of the production database and run the migration locally. What do you think would be the best approach? I'm open to doing both.

[danlipert]

@e18r Nice! Appreciate the extra care taken here :) Code is looking nice, if you can add some unit tests I'll add a tip to the bounty.

The strategy that I would take to write the tests is to first create some model objects in the database for grants and subscriptions manually that demonstrate the previous state of the app (i.e. no activity data). Then create some grants and subscriptions through the app itself - either call the underlying helper functions or create mock request objects and call the views individually. This will generate activity data, representing the current state of the app. Then do some asserts showing that there are no duplicates, and that each subscription, cancellation, etc. has the appropriate activity data. You could even have some tests that show what edge cases the helper functions are solving (i.e. close_enough).

[e18r]

@danlipert I've been working on the unit test. The hard part is getting the migration to run for the test, which is done with something called MigrationExecutor (see link in my previous post), but it has given me plenty of errors.

In order to solve them, I had to modify Gitcoin's settings.py to add 'migrations' in the INSTALLED_APPS, and also to include the MIGRATION_MODULES dict. Now I'm getting an unexplained KeyError when I call the migrate function from the executor.

I'll keep you posted on my progress. Any help appreciated.

[e18r]

@danlipert After a lot of work, I located the source of the issue as coming from pytest. When I use manage.py test, the KeyError doesn't show up.

However, another, more troubling error appears:

django.db.migrations.exceptions.IrreversibleError: Operation <RunPython <function generate_activities at 0x7fbf174eb268>> in dashboard.0032_data_for_old_grants is not reversible

In order to go back to a database state in which the migration wasn't applied, django needs to revert the migration, as explained here. However, it doesn't really make sense to write a reversal method by which I delete all the grant-related activities from a certain date in the past, does it?

Please consider employing a more ad-hoc test with production data because I reached a hard limit here.

[e18r]

@danlipert @thelostone-mc @owocki What are your thoughts on this?

[danlipert]

Hey @e18r - I'm not sure I understand - why is Django trying to run a reverse migration? Maybe instead of running the migration directly, you can have the migration call a helper function, and then in your test just call the helper function instead of the migration itself?

[e18r]

@danlipert I have a few questions:

• Do you want me to have the helper function outside the migration file, perhaps in utils.py? This could have potentially disastrous effects if someone modifies that helper function in the future.
• Do you want me to have the helper function inside the migration file? In that case: how can I import that helper function from the test file? from dashboard.migrations.0032_data_for_old_grants import helper doesn't seem to work.

[danlipert]

@e16r ah yes, I think the underscores may cause python's importing to fail :/ I'm thinking you can create a file inside the migrations directory like migrations/utils.py. Then put your function in there and you should be able to from dashboard.migrations import utils then utils.myfunction()

[e18r]

@danlipert when I create the file migrations/utils.py, I get the following error:

django.db.migrations.exceptions.BadMigrationError: Migration utils in app dashboard has no Migration class

Django thinks I created a new migration and expects a Migration class inside utils.py.

[danlipert]

@danlipert when I create the file migrations/utils.py, I get the following error:

django.db.migrations.exceptions.BadMigrationError: Migration utils in app dashboard has no Migration class

Django thinks I created a new migration and expects a Migration class inside utils.py.

@e18r Ah, my bad - you'll have to put the helper file elsewhere then

[e18r]

@danlipert This PR is ready for review and merge:

• a data migration was introduced
• two failsafe mechanisms prevent the insertion of duplicate data
• a comprehensive unit test suite prevents any bugs

[danlipert]

Looking great @octavioamu @thelostone-mc what do y'all think? @e18r can you squash and rebase? Thanks y'all!

[thelostone-mc]

lgtm ^_^

[e18r]

@danlipert @thelostone-mc @octavioamu I solved migration dependencies, squashed and rebased. The squash dismissed your reviews, I think, so you probably need to approve them again.