708709 - Affiliates: Adjust banner URL on "my banners" to specify CDN

Status: VERIFIED WONTFIX

(Firefox Affiliates Graveyard :: affiliates.mozilla.org, defect)

Description

[Chris More [:cmore]]

Currently, the banners listed on "my banners" are being pulled locally. Even though the CDN is just apache magic and not really a CDN, we should not be exposing banners to users with URLs that are not on the CDN to future-proof the location of the banners.

The banner img URLs on "my banners" should be the same as the URLs shown when creating a new banner.

Comment 1

[Osmose [:osmose, :mkelly]]

Moving to 1.2 milestone.

Comment 2

[Osmose [:osmose, :mkelly]]

Milos: Use the cdn argument for absolutify when the banner code is generated (check out apps/shared/utils.py for info on absolutify). The CDN_DOMAIN setting should already be set properly on our servers, so you can just define a test value in local.py to make sure it works.

Comment 3

[Milos Dinic [:Milos]]

Mkelly, does this look ok? https://github.com/milossh/affiliates/commit/f2a76a2edc16a78dfd2e29ddc7522e4a4c1cc878 

From now on, should I just ask for pull request so we can discuss it on github, or should I attach patches here and ask you/rik for a review?

Comment 4

[Osmose [:osmose, :mkelly]]

It looks right, but you should add a test to make sure that the URLs in the embed code includes the CDN url. Specifically, you should mock out the CDN_DOMAIN setting to some random string, and then check if a banner's code includes that CDN_DOMAIN in it.

As for process, we usually do code discussion in pull requests as you suggest. Once you have the test written, make a pull request and I'll review and merge it.

Comment 5

[Osmose [:osmose, :mkelly]]

As per bug 746219 we're not using a separate domain for the CDN anymore.

Comment 6

[Matt Brandt [:mbrandt]]

QA verified wontfix