HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythondjangoMinor

Storing folders of list of records in Google Drive

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
googlerecordsfoldersdriveliststoring

Problem

I've written a script using the Django ORM to store 'configuration settings' and interacting with the Google Drive API and another API. The script creates a base project folder and subfolders in Google Drive for a list of records, and posts links back to the source database. I'm a beginner in Python, so I'm not sure if I'm doing things the right way, or if it's completely terrible.

I think there are some areas that could be cleaned up -- for example, how I am creating the 'client' and 'service' objects repeatedly inside specific methods. I wasn't sure if it would work to create the objects and pass them between the methods though. I think I could also use the Google 'batch' and 'partial response' methods to optimize HTTP requests.

Also, I need to get this thing on a periodic task or crontab. I was interested in trying celery, so I initially set up the below jobs, and they were creating the clients, but I don't think it's working anymore in Celery.

I removed some of the script and commented areas to make it more understandable.

```
# Celery tasks
@periodic_task(run_every=crontab(hour="", minute="", day_of_week="*"))
def sync_folders():
configs = Configuration.objects.all()
for config in configs:
user = config.user
client = create_client(user, config)
try:
create_document_folders(config, client)
except Exception as inst:
pass

@periodic_task(run_every=crontab(hour="", minute="5", day_of_week=""))
def sync_files():
configs = Configuration.objects.all()
for config in configs:
user = config.user
client = create_client(user, config)
try:
get_updated_files(config, client)
except Exception as inst:
pass

# Rest of post is the script initiated by celery tasks above.
def create_client(user, config):
# Create the QuickBase Client object
s = config
if s is not None:
if s.qb_realm is not None:
if s.qb_realm != 'w

Solution

# Celery tasks
@periodic_task(run_every=crontab(hour="*", minute="*", day_of_week="*"))
def sync_folders():
    configs = Configuration.objects.all()
    for config in configs:
        user = config.user
        client = create_client(user, config)


I'd do: create_client(config.user, config)

try:
            create_document_folders(config, client)
        except Exception as inst:
            pass


Don't do this. You catch an exception and ignore it. You have no clue what went wrong. You should at the very least print out the exception, or put it in a log, or something so you can figure out what what goes wrong.

@periodic_task(run_every=crontab(hour="*", minute="5", day_of_week="*"))
def sync_files():
    configs = Configuration.objects.all()
    for config in configs:
        user = config.user
        client = create_client(user, config)
        try:
            get_updated_files(config, client)
        except Exception as inst:
            pass


Pretty the same function again. Refactor them and pass get_updated_files or create_document_folder as a parameter to the new function.

# Rest of post is the script initiated by celery tasks above. 
def create_client(user, config):
    # Create the QuickBase Client object
    s = config


Why? Just use config. The only advantage to s is that it's harder to read.

if s is not None:


Do you really want to support config being None?

if s.qb_realm is not None:
            if s.qb_realm != 'www':
                baseurl = 'https://' + s.qb_realm + '.quickbase.com'


Generally its better to use string formatting than concatenation.

else:
                baseurl = 'https://www.quickbase.com'


Why is this a special case? it just does the same thing that the above case would do.

else:
            baseurl = 'https://www.quickbase.com'

        client = quickbase.Client(QUICKBASE_USER, QUICKBASE_PASSWORD, base_url=baseurl)
        return client


Combine those two lines

def get_jobs_wo_folders(config, client):
    if client is not None:


Having a function silently fail to do its job given bad input is a bad idea. You should throw an exception. I wouldn't even bother to check if client were None, and just let it fail when it tries to use it.

query = "{'" + config.root_folder_id + "'.EX.''}"
        clist = [config.root_folder_id, config.root_folder_name_fid, config.root_folder_user, '3']
        records = client.do_query(query, columns=clist, database=config.qb_root_dbid)
        return records

def retrieve_specific_files(service, param):
    # Search and retrieve a list of file resources passing a params object
    result = []
    page_token = None
    while True:
        try:
            if page_token:


Use is not None to check for None, just because a few other random things end up being false and its more explicit this way

param['pageToken'] = page_token
            files = service.files().list(**param).execute()
            result.extend(files['items'])
            page_token = files.get('nextPageToken')
            if not page_token:
                break
        except errors.HttpError, error:
            print 'An error occurred: %s' % error
            break


Do you really want to try and continue on if there is an error? Shouldn't you just give up this attempt?

return result

def get_csv(list):


avoid list as a variable name since its a builtin python type

si = cStringIO.StringIO()
    cw = csv.writer(si)
    cw.writerows(list)
    return si.getvalue().strip('\r\n')

# Google helper functions
def refresh_access_token(user, config):
    instance = UserSocialAuth.objects.get(user=user, provider='google-oauth2')
    instance.refresh_token()
    # Configuration model has a field 'last_refresh_time' that is auto-now date/time
    config.save()
    return instance.extra_data['access_token']

def get_access_token(config):
    try:
        access_token = UserSocialAuth.objects.get(user=config.user, provider='google-oauth2').extra_data['access_token']


You just did something very similiar, this suggests that you should extract a common function

return access_token
    except Exception as inst:
        pprint(inst)


At least here you print the exception.

def create_drive_service(config):
"""
Creates a drive service using the 'Configuration' object's
related user. Users initial signup is done with django-socialauth;
the access token and refresh token is saved with django-socialauth &
refresh token handling is done with django-socialauth.
"""
    c = config
    user = config.user


Why do you sometimes use config.user and other times pass the user as a parameter?

instance = UserSocialAuth.objects.get(user=user, provider='google-oauth2')


avoid generic names like instance

```
refreshed_at = c.token_last_refresh
now = datetime.now()
expires_in = instance.extra_data['expires']
token_age = (now - refreshed_at).seconds

Code Snippets

# Celery tasks
@periodic_task(run_every=crontab(hour="*", minute="*", day_of_week="*"))
def sync_folders():
    configs = Configuration.objects.all()
    for config in configs:
        user = config.user
        client = create_client(user, config)
try:
            create_document_folders(config, client)
        except Exception as inst:
            pass
@periodic_task(run_every=crontab(hour="*", minute="5", day_of_week="*"))
def sync_files():
    configs = Configuration.objects.all()
    for config in configs:
        user = config.user
        client = create_client(user, config)
        try:
            get_updated_files(config, client)
        except Exception as inst:
            pass
# Rest of post is the script initiated by celery tasks above. 
def create_client(user, config):
    # Create the QuickBase Client object
    s = config
if s is not None:

Context

StackExchange Code Review Q#21597, answer score: 2

Revisions (0)

No revisions yet.