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

Server changes needed for view time #418

Merged
merged 4 commits into from
Aug 11, 2021

Conversation

davidfischer
Copy link
Collaborator

This adds the server changes needed to account for view time sent from the client in readthedocs/ethical-ad-client#76.

Note: these changes include a migration that adds a column to the Offer table. This should be run with extreme care in prod.

Fixes #359 from a data capture perspective but the data still needs to be put into reports and such. We should probably collect data for a while before doing that.

Note: these changes include a migration that adds a column
  to the Offer table. This should be run with extreme care in prod.
@davidfischer davidfischer requested a review from a team July 20, 2021 22:22
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

Simple enough change. The migration definitely scares me, though maybe it won't be blocking, since it's nullable?

adserver/models.py Show resolved Hide resolved
adserver/views.py Outdated Show resolved Hide resolved
adserver/views.py Outdated Show resolved Hide resolved
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

Changes look good 👍

Sidenote: are we currently using the send_to_analytics function? I don't think we are -- might make sense to remove it.

@@ -722,6 +734,35 @@ def get_response(self, request, advertisement, publisher):
return HttpResponseRedirect(url)


class AdViewTimeProxyView(AdViewProxyView):
Copy link
Member

Choose a reason for hiding this comment

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

What are we using from the AdViewProxyView that requires subclassing? Seems we're removing most of the logic?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Mostly just getting the relevant offer, publisher, and ad.

@davidfischer
Copy link
Collaborator Author

Sidenote: are we currently using the send_to_analytics function? I don't think we are -- might make sense to remove it.

We were at one point but I don't believe we are currently.

@davidfischer davidfischer merged commit 786308c into main Aug 11, 2021
@davidfischer davidfischer deleted the davidfischer/server-view-time-changes branch August 11, 2021 21:46
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.

Capture "view time" for ads
2 participants