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

Jk/hubspot sync #291

Closed
wants to merge 11 commits into from
Closed

Jk/hubspot sync #291

wants to merge 11 commits into from

Conversation

jklingen92
Copy link
Contributor

@jklingen92 jklingen92 commented May 15, 2019

This PR is waiting on 3 things:

  • Merge PR Registration form - Step 2 #236
  • Define hubpost contact properties on the mitxpro hubspot application
  • Decide whether to run a full contact sync at regular intervals or implement live updating. This could be done by extending the save method on the User object

Pre-Flight checklist

  • Screenshots and design review for any changes that affect layout or styling
    • Desktop screenshots
    • Mobile width screenshots
    • Tag @Ferdi or @pdpinch for review
  • Migrations
    • Migration is backwards-compatible with current production code
  • Testing
    • Code is tested
    • Changes have been manually tested
  • Settings
    • New settings are documented and present in app.json
    • New settings have reasonable development defaults, if applicable

What are the relevant tickets?

#275
#236

What's this PR do?

Add celery tasks to sync contacts with hubspot.

How should this be manually tested?

You could run it in ipython, make a change to a user, and then run the task manually as opposed to through celery. Then check that the update was made on Hubspot.

Where should the reviewer start?

users/tasks

Any background context you want to provide?

(Optional)

Screenshots (if appropriate)

(Optional)

What GIF best describes this PR or how it makes you feel?

Alt Text

@jklingen92 jklingen92 requested review from Ferdi and pdpinch May 15, 2019 18:24
@odlbot odlbot temporarily deployed to xpro-ci-pr-291 May 15, 2019 18:25 Inactive
@codecov-io
Copy link

codecov-io commented May 15, 2019

Codecov Report

Merging #291 into master will decrease coverage by 0.06%.
The diff coverage is 86.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #291      +/-   ##
==========================================
- Coverage   94.42%   94.35%   -0.07%     
==========================================
  Files         159      161       +2     
  Lines        5343     5388      +45     
  Branches      311      319       +8     
==========================================
+ Hits         5045     5084      +39     
- Misses        242      245       +3     
- Partials       56       59       +3
Impacted Files Coverage Δ
users/models.py 98.73% <ø> (ø) ⬆️
users/exceptions.py 100% <100%> (ø)
mitxpro/settings.py 92.73% <100%> (+0.04%) ⬆️
users/tasks.py 86.04% <86.04%> (ø)

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 fb796eb...b310730. Read the comment docs.

@odlbot odlbot temporarily deployed to xpro-ci-pr-291 May 15, 2019 18:31 Inactive
@odlbot odlbot temporarily deployed to xpro-ci-pr-291 May 15, 2019 18:32 Inactive
@Ferdi
Copy link

Ferdi commented May 15, 2019

@jklingen92 you should find a way to embed the gif , not just a link - too much work !

@odlbot odlbot temporarily deployed to xpro-ci-pr-291 May 15, 2019 19:04 Inactive
@jklingen92 jklingen92 requested a review from mbertrand May 15, 2019 19:04
@mbertrand mbertrand self-assigned this May 15, 2019
@odlbot odlbot temporarily deployed to xpro-ci-pr-291 May 15, 2019 19:16 Inactive
@odlbot odlbot temporarily deployed to xpro-ci-pr-291 May 15, 2019 19:26 Inactive
users/tasks.py Outdated Show resolved Hide resolved
users/tasks.py Outdated Show resolved Hide resolved
users/tasks_test.py Show resolved Hide resolved
@odlbot odlbot temporarily deployed to xpro-ci-pr-291 May 15, 2019 20:59 Inactive
@odlbot odlbot temporarily deployed to xpro-ci-pr-291 May 15, 2019 21:38 Inactive
@jklingen92 jklingen92 requested a review from mbertrand May 15, 2019 21:40
users/tasks.py Outdated
"value": mapping["default"] if "default" in mapping else "",
}
if mapping["model"] == "profile" and hasattr(user, "profile"):
prop["value"] = getattr(user.profile, mapping["field"])
Copy link
Member

Choose a reason for hiding this comment

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

This line doesn't seem to be covered by unit tests, can you add one? Probably need to create & use a ProfileFactory class and include it as part of UserFactory:
profile = RelatedFactory("users.factories.Profile", "user")

Not sure if it's relevant or not, but it seems like Hubspot contacts can have state/region and country fields, those would need to be obtained from the LegalAddress model if they need to be synced with Hubspot too.

Copy link
Contributor Author

@jklingen92 jklingen92 May 16, 2019

Choose a reason for hiding this comment

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

@mbertrand This is what I was asking about regarding the Profile Model. Should that be added as a part of this PR?

Copy link
Member

Choose a reason for hiding this comment

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

Yes if you need any of the fields stored in Profile to also be saved in Hubspot contacts, which seems likely. Ditto for LegalAddress. @pdpinch or @Ferdi which fields should be synced? Issue #275 doesn't explicitly specify any as far as I can see, but edx syncs the following: first name, last name, company, job title, state, country, gender, degree.

users/tasks.py Show resolved Hide resolved
users/tasks.py Outdated Show resolved Hide resolved
users/tasks.py Outdated Show resolved Hide resolved
users/tasks.py Show resolved Hide resolved
@mbertrand
Copy link
Member

Any update on these?:

Define hubpost contact properties on the mitxpro hubspot application
Decide whether to run a full contact sync at regular intervals or implement live updating. This could be done by extending the save method on the User object

@odlbot odlbot temporarily deployed to xpro-ci-pr-291 May 16, 2019 16:13 Inactive
@odlbot odlbot temporarily deployed to xpro-ci-pr-291 May 16, 2019 17:16 Inactive
@jklingen92
Copy link
Contributor Author

jklingen92 commented May 16, 2019

Any update on these?:
Define hubpost contact properties on the mitxpro hubspot application
Decide whether to run a full contact sync at regular intervals or implement live updating. This could be done by extending the save method on the User object

@Ferdi @pdpinch I'm planning on imitating the Profile model and extending the profile save function.

@mbertrand
Copy link
Member

@jklingen92 should this PR be closed?

@jklingen92 jklingen92 added the Duplicate This issue or pull request already exists label May 22, 2019
@jklingen92 jklingen92 closed this May 22, 2019
@jklingen92 jklingen92 deleted the jk/hubspot-sync branch May 22, 2019 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate This issue or pull request already exists Waiting on Author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants