numberly / appnexus-client

:snake: General purpose Python client for the AppNexus API
https://appnexus-client.readthedocs.io
MIT License
39 stars 19 forks source link

Add reporting related services to create and download them #20

Closed mevinbabuc closed 6 years ago

mevinbabuc commented 6 years ago
from appnexus import connect, Report

connect("username", "password", test=True)

json = {
    "report_type": "insertion_order_frequency",
    "columns": [
        "hour",
        "imps",
        "clicks",
        "total_convs",
        "ctr",
        "convs_rate"
    ],
    "report_interval": "lifetime",
    "format": "csv"
}
create_report_response = Report(json).save()
Report.download(id=create_report_response['report_id'])
coveralls commented 6 years ago

Coverage Status

Coverage decreased (-1.0%) to 89.244% when pulling ed76014536c1921dd2a80b659a8918b661442d21 on mevinbabuc:master into 4f705c73690df6e35e2a7f9f569583039686ebda on numberly:master.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.1%) to 90.323% when pulling 4fa343f172e4ac1ba226b86f080f6c606a7c0a35 on mevinbabuc:master into 2d988178014fe07e9c6026f290ba84d79e6703ba on numberly:master.

ramnes commented 6 years ago

Could you please update your example so that it actually works? What's client.report_download? Also, I'd like to see how you get the id rather than entering it manually.

PS: sorry for the closing, that was a misclick.

ramnes commented 6 years ago

Okay so, following the discussion on #21, rather than implementing a get method inside Model, which as explained is not something we want to do, I'd like us to have (in model.py) something in the lines of:

class Report(Model):
    def download(self):
        self.client.get("ReportDownload", self.id)

(obviously I didn't test this and it probably won't work out of the box)

so that we could simply do:

# Create a report
report = Report(json).save()

# Download the report
report.download()

I feel that it's a much better interface. How does that sound to you? Can you make it that way?

ramnes commented 6 years ago

Then we would also remove ReportDownload from the services_list.

mevinbabuc commented 6 years ago

I have added a Report Model with a download method on it. I was expecting the .save() to return a report object, but it returned a dictionary.

mevinbabuc commented 6 years ago

If you want to achieve a syntax close to what you showed above; we'll have to change the save method on the model to return a model object rather than result.

    def save(self, **kwargs):
        payload = self.__dict__
        if "id" not in self.__dict__:
            logger.info("creating a {}".format(self.service))
            result = self.create(payload, **kwargs)
        else:
            result = self.modify(payload, id=self.id, **kwargs)
        return type(self)(result)

returning a type(self)(result) can achieve that. Then we can modify the Report Model to something like this

class Report(Model):

    def download(self, **kwargs):
        return self.client.get("report-download", id=self.report_id)

with which you'll be able to do

# Create a report
report = Report(json).save()

# Download the report
report.download()

Is the change to the save method to return a model object welcomed ?

mevinbabuc commented 6 years ago

I have the PR for that change ready; Let me know and I can raise one with that change

ramnes commented 6 years ago

Rather than instantiating a new object, you can simply use Thingy.update and do self.update(result).

But yes, definitely, that would be awesome, please open that other PR! :+1:

Looks like this is something we forgot when migrating to Thingy.