-
Notifications
You must be signed in to change notification settings - Fork 12
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
ROI.COM.AU’s latest changes (AdWords compatibility, plus other changes) #4
Open
chris-morgan
wants to merge
66
commits into
alexhayes:master
Choose a base branch
from
roi-com-au:master
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
release/0.7.2 0.7.2
No code changes required for this particular upgrade. This should buy us a couple of months.
I *think* these changes actually take it to compatibility with v201603, but I haven’t tested it yet. - The estimate fields are gone in favour of some “all conversions” fields that I don’t care about. ROI.COM.AU aren’t using any of this stuff, so let’s just throw it away. - The conversion-many-clicks fields became the new just-plain-conversion fields. Same display names for the fields, so no code changes required beyond updating the report definition.
The AdWords API doesn’t suggest that the `entries` attribute will be missing, but it is indeed lacking in at least some responses when there are zero entries (e.g. a `TargetingIdeaPage` response where you asked for keyword suggestions but Google didn’t have any to offer). So, in such a case we just don’t yield any elements, rather than raising an `AttributeError`.
Insufficient data is not always a problem. If your access has been revoked, you may just wish to ignore the fact.
For members of the public API the old names have been left around as aliases, so there should not be any compatibility problems. Unless you were doing things like hardcoding the name of an exception as a string. Which is, I suppose, quite feasible. Still, I’m not going to consider it a breaking change.
Its display name was changed from “Max. CPA (converted clicks)” to “Target CPA” in v201601 and ROI.COM.AU isn’t using it at all (and is in fact heading away from using these metrics models), so I’m ditching the field rather than renaming the field from `max_cpa_converted_clicks` to `target_cpa`.
Formerly it was a part of the report definition; this is finally removed in favour of the HTTP header approach. This is a breaking change. If you had `'includeZeroImpressions'` in your report definition for a `ReportFile.objects.request()` call you will need to remove it from the report definition and start passing it as a keyword argument named `include_zero_impressions` instead.
One notable change in v201603 not covered by previous commits is that empty values become '--' (in my experience, maybe '-- '?) rather than an empty string. This may affect user code.
As I’m bumping 0.7.7 to 0.8.0, these can disappear.
release/0.8.2 0.8.2
…me/jstorer/workspace/django-google-adwords/CHANGELOG.md
release/0.8.3 0.8.3
auvipy
approved these changes
Apr 12, 2017
…moved as of version V201702
…me/jstorer/workspace/django-google-adwords/CHANGELOG.md
release/0.8.4 0.8.4
…me/jstorer/workspace/django-google-adwords/CHANGELOG.md
release/0.8.5 0.8.5
… returning doubles
…me/jstorer/workspace/django-google-adwords/CHANGELOG.md
release/0.8.6 0.8.6
…me/jstorer/workspace/django-google-adwords/CHANGELOG.md
release/0.8.7 0.8.7
…me/jstorer/workspace/django-google-adwords/CHANGELOG.md
release/0.8.8 0.8.8
…me/jstorer/workspace/django-google-adwords/CHANGELOG.md
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note that because we weren’t using all of the fields in the models at ROI.COM.AU certain changes which could have been made non-destructively (renaming fields) were instead made destructively (removing them), because I was lazy and we’re moving away from the caching-metrics approach where convenient (i.e. for new things). I explained it in the commit messages. There are also some changes you may disagree with or not want. Anyway, here it is, to merge or not, as desired.