Fix of TypeError: string indices must be integers, not str

[LeMeteore]

The get_country_statistic("") method is returning a string. To access the 'localitities' key, we should first convert the string to a python dictionary

•    context['locality_count'] = get_country_statistic("")['localities']
  

•   context['locality_count'] = json.loads(get_country_statistic(""))['localities']
  

[meomancer]

LGTM

[dodobas]

this indeed is an error, however we should focus on fixing the main cause of this error

seems like the root cause of this error is that the CLUSTER_CACHE_DIR is not created, so the get_country_statistic will eventually end up at https://github.com/healthsites/healthsites/blob/develop/django_project/localities/utils.py#L129 returning an empty output

as a quick fix, maybe we could just create the CLUSTER_CACHE_DIR in the last block try block

ideally, we would probably do something like:

• update developer/deploy documentation on how to deploy the local version (which Django management commands to execute after migrate)
• check basic project requirements using Django system check framework (https://docs.djangoproject.com/en/1.8/ref/checks/)

[LeMeteore]

this indeed is an error, however we should focus on fixing the main cause of this error

I agree, I sent this quick fix, as I was trying to quickly send this fix.

ideally, we would probably do something like:

• update developer/deploy documentation on how to deploy the local version (which Django management commands to execute after migrate)
• check basic project requirements using Django system check framework (https://docs.djangoproject.com/en/1.8/ref/checks/)

I would be glad to work on that point if you give me enough pointers on how to proceed, what to update/change, etc... And talking about deploying a local version of the project, I tried this. You'll let me know what you think.

[dodobas]

for this pull request you could just update it in a way to create CLUSTER_CACHE_DIR in the last block try block, and with this fix the project so it will at least 'open'

for other issues we should connect in the https://gitter.im/healthsites/healthsites and have a broader discussion about the development/deployment setup in the project

[LeMeteore]

This is where I am:

updating the file django_project/localities/utils.py to create the missing directory if necessary:

       else:
            # get cache                                                                                                                                
            filename = os.path.join(
                settings.CLUSTER_CACHE_DIR,
                'world_statistic'
            )
            try:
                file = open(filename, 'r')
                data = file.read()
                output = json.loads(data)
            except IOError as e:
                try:
                    # cluster cache directory does not exists, create it
                    os.makedirs(settings.CLUSTER_CACHE_DIR)   
                                                                                        
                    # query for each of attribute                                                                                                      
                    healthsites = get_heathsites_master()
                    output = get_statistic(healthsites)
                    output = json.dumps(output, cls=DjangoJSONEncoder)
                    file = open(filename, 'w')
                    file.write(output)  # python will convert \n to os.linesep                                                                         
                    file.close()  # you can omit in most cases as the destructor will call it                                                          
                    output = json.loads(output)
                except Exception as e:
                    pass

You mentioned the idea of using Django system checks, so I tried this:

1. Addition of django_project/localities/checks.py as:

import os                                                                                                                                              
from django.core.checks import register, Tags, Info
from django.conf import settings

@register(Tags.compatibility)
def check_cluster_cache_dir_is_installed(app_configs, **kwargs):
    "Check that cluster cache dir is created."
    checks = []
    if not os.path.exists(settings.CLUSTER_CACHE_DIR):
        msg = ("The cluster cache directory does not exists. This folder is necessary for ... "
               "You can create one from the command line as eg: `mkdir cache`.")
        checks.append(Info(msg, id='localities.I001'))                                                                                                                      

    return checks

2. Update of django_project/localities/__init__.py as:

from __future__ import absolute_import

# This will make sure the app is always imported when                                                                                                  
# Django starts so that shared_task will use this app.                                                                                                 
from .celery import app as celery_app
from . import checks

The displayed warning will be like:

Performing system checks...

System check identified some issues:

INFOS:
?: (localities.I001) The cluster cache directory does not exists. This folder is necessary for ... You can create one from the command line as eg: `mkdir cache`.

System check identified 1 issue (0 silenced).
May 13, 2017 - 12:02:21
Django version 1.8.18, using settings 'core.settings.dev_nsukami'
Starting development server at http://127.0.0.1:8000/
Quit the server with CONTROL-C.

I guess, the 2 can't go togheter. If we create the cluster cache directory in case it's missing, there is no need for the system check. And if we do the system check, we should let the user create the cluster cache directory himself.

What do you think?

[dodobas]

you are on the right track, but maybe we should be careful when creating the directory, the statement os.makedirs will raise an Error exception if the directory already exists (https://docs.python.org/2.7/library/os.html#os.makedirs)
it would be much better to check if the directory exists:

if not os.path.exists(settings.CLUSTER_CACHE_DIR):
    os.makedirs(settings.CLUSTER_CACHE_DIR)

Regarding your other comments, I would first like to have a broader discussion on how to handle basic project setup and update the documentation. So for this pull request just focus on fixing the initial problem and don't add anything else.

[LeMeteore]

Haha, I was on the way :) https://github.com/healthsites/healthsites/pull/927/files

[dodobas]

fixed by #927