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

sync email subscriber and profiles keywords field #4474

Merged
merged 5 commits into from
Jun 1, 2019

Conversation

owocki
Copy link
Contributor

@owocki owocki commented May 23, 2019

Description

sync email subscriber and profiles keywords field per #4424

Refers/Fixes

#4424

Testing

tested on my local

@codecov
Copy link

codecov bot commented May 23, 2019

Codecov Report

Merging #4474 into master will decrease coverage by 0.05%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4474      +/-   ##
==========================================
- Coverage   30.08%   30.03%   -0.06%     
==========================================
  Files         209      210       +1     
  Lines       16850    16881      +31     
  Branches     2267     2275       +8     
==========================================
  Hits         5070     5070              
- Misses      11582    11613      +31     
  Partials      198      198
Impacted Files Coverage Δ
...p/dashboard/management/commands/sync_es_profile.py 0% <0%> (ø)

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 d59ae5c...c371c8b. Read the comment docs.

@codecov
Copy link

codecov bot commented May 23, 2019

Codecov Report

Merging #4474 into master will decrease coverage by 0.04%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4474      +/-   ##
==========================================
- Coverage   30.17%   30.12%   -0.05%     
==========================================
  Files         209      210       +1     
  Lines       16886    16914      +28     
  Branches     2278     2285       +7     
==========================================
  Hits         5095     5095              
- Misses      11594    11622      +28     
  Partials      197      197
Impacted Files Coverage Δ
...p/dashboard/management/commands/sync_es_profile.py 0% <0%> (ø)

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 4ec5cca...2e8fb5c. Read the comment docs.

@SaptakS SaptakS force-pushed the kevin/sync_keyword_profile branch from c371c8b to b58816e Compare May 23, 2019 21:24
@owocki
Copy link
Contributor Author

owocki commented May 23, 2019

its not clear to me that a cronjob is the right place for this... maybe the profile <> emailsubscriber sync could happpen in a presave hook.

@SaptakS
Copy link
Contributor

SaptakS commented May 24, 2019

I was thinking more of a post_save hook in both the models.

Copy link
Contributor

@danlipert danlipert left a comment

Choose a reason for hiding this comment

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

One question but other than that looks good

es = EmailSubscriber.objects.filter(email=profile.email).exclude(keywords=[]).first()
if es:
print("2", es.keywords, profile.handle)
profile.keywords = es.keywords
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we want to append these keywords rather than overwrite them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@owocki
Copy link
Contributor Author

owocki commented May 30, 2019

just addressed dan's feedback

profile.save()

# pull keywords by emailsubscriber into profile
for profile in Profile.objects.filter(keywords=[]):
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we are iterating over the profiles that don't have any keywords, but the for loop above assigns keywords to each profile that lacks them if there is a search history object for that user. In this loop, we are combining the email subscriber keywords with the profile keywords, but we are only iterating over profiles that have no keywords.

I think we need to combine the two for profile in Profile.objects.filter(keywords=[]): loops so we can get both search history and email subscriber keywords applied. In the current code, adding the search history keywords to a profile essentially skips adding email subscriber keywords to that same profile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danlipert danlipert merged commit e6257b3 into master Jun 1, 2019
@thelostone-mc thelostone-mc deleted the kevin/sync_keyword_profile branch July 4, 2019 14:54
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