-
-
Notifications
You must be signed in to change notification settings - Fork 775
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
WOC: Implement utm* params of a login UserAction stored in the DB feature #2212 #2359 #2378
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2378 +/- ##
=========================================
- Coverage 29.47% 29.27% -0.2%
=========================================
Files 146 148 +2
Lines 11777 11946 +169
Branches 1599 1621 +22
=========================================
+ Hits 3471 3497 +26
- Misses 8188 8324 +136
- Partials 118 125 +7
Continue to review full report at Codecov.
|
@owocki I believe this is ready for a reviewer. |
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.
code LGTM.. didnt test tho..
cc @mbeacom
hi @mbeacom |
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.
Looks good! ...nice work. Couple comments/questions.
|
||
return {key, value}; | ||
}) | ||
.filter((param) => [ 'utm_medium', 'utm_source', 'utm_campaign' ].indexOf(param.key) !== -1) |
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.
Nit - can we use includes
here?
.filter((param) => [ 'utm_medium', 'utm_source', 'utm_campaign' ].indexOf(param.key) !== -1) | |
.filter((param) => [ 'utm_medium', 'utm_source', 'utm_campaign' ].includes(param.key)) |
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.
you no need to support IE?
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.
Hmm....I can see babel in the devDependencies...and it looks like it's used in the Webpack config (but I'm not a Webpack expert, TBH) so I think IE should be supported.
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.
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.
Ahhh....fair enough. Suppose babel config could be updated...but likely out of scope for this PR.
@staticmethod | ||
@patch('dashboard.utils.UserAction.objects') | ||
def test_create_user_action_without_cookie(mockUserAction): | ||
"""Test the giving utm* in cookie should store in DB.""" |
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.
Nit - comment needs updating
@staticmethod | ||
@patch('dashboard.utils.UserAction.objects') | ||
def test_create_user_action_campaign_not_json(mockUserAction): | ||
"""Test the giving utm* in cookie should store in DB.""" |
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.
Nit - comment needs updating
|
||
@staticmethod | ||
@patch('dashboard.utils.UserAction.objects') | ||
def test_create_user_action_campaign_not_json(mockUserAction): |
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.
How is this test different then test_create_user_action_with_cookie
?
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.
I want to test partial value in cookie scenario...
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.
Gotcha.
Does it make sense to rename the test method to something like test_create_user_action_with_partial_cookie
?
Description
The 3rd times create same pull request...
This PR is for saving utm* params to DB for data analysis, relevant story is 2212.
Whenever user access gitcoin with "utm_medum, utm_source, utm_campaign" will store them in cookie, after user login or user logout will store to dashboard_useraction table.
Checklist
Affected core subsystem(s)
UserAction data model.
Testing
Test with below 3 scenarios.
Scenario: Record utm* in Cookie
Give: utm_medum, utm_source, utm_campaign in URL
When: User open gitcoin
Then: Should record utm* in Cookie
Scenario: Record utm* in DB
Give: utm* in Cookie
When: User click "LOG IN" button
Then: Should record utm* in DB
Scenario: Record utm* in DB
Give: utm* in Cookie
When: User click "Log Out" button
Then: Should record utm* in DB
Refers/Fixes