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 ecommerce sync #319

Merged
merged 1 commit into from
May 21, 2019
Merged

Jk/hubspot ecommerce sync #319

merged 1 commit into from
May 21, 2019

Conversation

jklingen92
Copy link
Contributor

@jklingen92 jklingen92 commented May 20, 2019

Some change to User in this PR is causing 2 authentication pipeline tests to fail. I need to look into this.

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?

#276 #18 #291

What's this PR do?

Add management command configure_hubspot_bridge to install and manage hubspot ecommerce bridge, add hubspot_api.py with utilities for making requests to hubspot

How should this be manually tested?

Use the hubspot API and the management command to configure hubspot settings.

Where should the reviewer start?

(Optional)

Any background context you want to provide?

Screenshots (if appropriate)

(Optional)

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

(Optional)

@jklingen92 jklingen92 requested a review from mbertrand May 20, 2019 21:31
@odlbot odlbot temporarily deployed to xpro-ci-pr-319 May 20, 2019 21:31 Inactive
@jklingen92 jklingen92 requested review from Ferdi and pdpinch May 20, 2019 21:31
@codecov-io
Copy link

codecov-io commented May 21, 2019

Codecov Report

Merging #319 into master will decrease coverage by 0.18%.
The diff coverage is 81.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #319      +/-   ##
==========================================
- Coverage   94.22%   94.03%   -0.19%     
==========================================
  Files         168      170       +2     
  Lines        5695     5771      +76     
  Branches      330      342      +12     
==========================================
+ Hits         5366     5427      +61     
- Misses        270      280      +10     
- Partials       59       64       +5
Impacted Files Coverage Δ
ecommerce/tasks.py 100% <100%> (ø)
mitxpro/settings.py 93.01% <100%> (+0.03%) ⬆️
users/serializers.py 89.76% <50%> (-5.93%) ⬇️
ecommerce/hubspot_api.py 84.44% <84.44%> (ø)

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 6f051c5...602fb80. Read the comment docs.

@odlbot odlbot temporarily deployed to xpro-ci-pr-319 May 21, 2019 13:47 Inactive
ecommerce/hubspot_api.py Outdated Show resolved Hide resolved
ecommerce/hubspot_api_test.py Show resolved Hide resolved
ecommerce/hubspot_api.py Outdated Show resolved Hide resolved
ecommerce/hubspot_api_test.py Outdated Show resolved Hide resolved
ecommerce/hubspot_api.py Outdated Show resolved Hide resolved
ecommerce/hubspot_api.py Outdated Show resolved Hide resolved
ecommerce/hubspot_api.py Outdated Show resolved Hide resolved
ecommerce/tasks.py Outdated Show resolved Hide resolved
@odlbot odlbot temporarily deployed to xpro-ci-pr-319 May 21, 2019 14:41 Inactive
@odlbot odlbot temporarily deployed to xpro-ci-pr-319 May 21, 2019 15:21 Inactive
@jklingen92 jklingen92 requested a review from mbertrand May 21, 2019 15:28
users/models.py Outdated Show resolved Hide resolved
ecommerce/tasks.py Outdated Show resolved Hide resolved
ecommerce/tasks.py Outdated Show resolved Hide resolved
ecommerce/tasks.py Outdated Show resolved Hide resolved
ecommerce/tasks.py Show resolved Hide resolved
users/models.py Outdated Show resolved Hide resolved
@odlbot odlbot temporarily deployed to xpro-ci-pr-319 May 21, 2019 15:55 Inactive
ecommerce/hubspot_api.py Outdated Show resolved Hide resolved
ecommerce/tasks.py Outdated Show resolved Hide resolved
@jklingen92
Copy link
Contributor Author

@mbertrand The hubspot link you sent won't open. It seems to be infinitely appending an id into the URL...

What hubspot app are you testing with? Have you run the management command to configure the bridge?

@odlbot odlbot temporarily deployed to xpro-ci-pr-319 May 21, 2019 17:26 Inactive
@odlbot odlbot temporarily deployed to xpro-ci-pr-319 May 21, 2019 17:59 Inactive
@odlbot odlbot temporarily deployed to xpro-ci-pr-319 May 21, 2019 18:47 Inactive
@odlbot odlbot temporarily deployed to xpro-ci-pr-319 May 21, 2019 18:51 Inactive
ecommerce/hubspot_api_test.py Outdated Show resolved Hide resolved
@odlbot odlbot temporarily deployed to xpro-ci-pr-319 May 21, 2019 19:05 Inactive
Copy link
Member

@mbertrand mbertrand left a comment

Choose a reason for hiding this comment

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

Looks great! 👍
I don't recall if this will be your first PR merge or not, in case it is please squash all commits into one before merging.

Initial task files

Add ecommerce bridge sync

Add product mapping, sync in user serializer

Add management command to configure settings, add stubs for other models

Run black, fix UserSerializer / ProfileSerializer interaction

Fix parameter error and simplify tests

Fix fake data, add urllib, stub model id params

Fix function calls

Add unit tests for task functions

Remove created_on from sync, fix urls

Url bug

Fix test parametrization
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants