Repository: EdinburghGenomics/EGCG-Core

Issue:
clarity.get_list_of_samples crashes if a sample doesn't exist in the Lims
The error is because of the way we're processing sample names. The stacktrace is misleading:
```
File "clarity.py", line 98, in get_list_of_samples
    results.extend(_get_list_of_samples(sample_names[start:start+max_query]))
  File "clarity.py", line 114, in _get_list_of_samples
    samples.extend(_get_list_of_samples(remainder, sub + 1))
  File "clarity.py", line 114, in _get_list_of_samples
    samples.extend(_get_list_of_samples(remainder, sub + 1))
  File "clarity.py", line 114, in _get_list_of_samples
    samples.extend(_get_list_of_samples(remainder, sub + 1))
  File "clarity.py", line 102, in _get_list_of_samples
    pattern, repl = substitutions[sub]
```
We should return None for non-existent samples, or leave them out of the returned list.

Discussion:

Patch:
diff --git a/egcg_core/__init__.py b/egcg_core/__init__.py
index e1424ed..58d168b 100644
--- a/egcg_core/__init__.py
+++ b/egcg_core/__init__.py
@@ -1,1 +1,1 @@
-__version__ = '0.3.1'
+__version__ = '0.4'
diff --git a/egcg_core/clarity.py b/egcg_core/clarity.py
index 4691f7c..e896d04 100644
--- a/egcg_core/clarity.py
+++ b/egcg_core/clarity.py
@@ -2,13 +2,16 @@ import re
 from genologics.lims import Lims
 from egcg_core.config import cfg
 from egcg_core.app_logging import logging_default as log_cfg
-from egcg_core.exceptions import LimsCommunicationError
+from egcg_core.exceptions import EGCGError
 
 app_logger = log_cfg.get_logger('clarity')
 try:
     from egcg_core.ncbi import get_species_name
 except ImportError:
-    app_logger.error('Could not import egcg_core.ncbi. Is sqlite3 available?')
+    app_logger.warning('Could not import egcg_core.ncbi. Is sqlite3 available?')
+
+    def get_species_name(query_species):
+        raise EGCGError('Could not import egcg_core.ncbi.get_species_name - sqlite3 seems to be unavailable.')
 
 
 _lims = None
@@ -91,8 +94,8 @@ def sanitize_user_id(user_id):
 
 substitutions = (
     (None, None),
-    (re.compile('_(\d{2})$'), ':\g<1>'),
-    (re.compile('__(\w):(\d{2})'), ' _\g<1>:\g<2>')
+    (re.compile('_(\d{2})$'), ':\g<1>'),  # '_01' -> ':01'
+    (re.compile('__(\w):(\d{2})'), ' _\g<1>:\g<2>')  # '__L:01' -> ' _L:01'
 )
 
 
@@ -115,11 +118,12 @@ def _get_list_of_samples(sample_names, sub=0):
     lims.get_batch(samples)
 
     if len(samples) != len(sample_names):  # haven't got all the samples because some had _01/__L:01
+        sub += 1
+        remainder = sorted(set(_sample_names).difference(set([s.name for s in samples])))
         if sub < len(substitutions):
-            remainder = sorted(set(_sample_names).difference(set([s.name for s in samples])))
-            samples.extend(_get_list_of_samples(remainder, sub + 1))
-        else:
-            raise LimsCommunicationError('Expected %s back, got %s' % (_sample_names, len(samples)))
+            samples.extend(_get_list_of_samples(remainder, sub))
+        else:  # end recursion
+            app_logger.warning('Could not find %s in Lims' % remainder)
 
     return samples
 
diff --git a/egcg_core/executor/cluster_executor.py b/egcg_core/executor/cluster_executor.py
index ce66a44..f9b29d1 100644
--- a/egcg_core/executor/cluster_executor.py
+++ b/egcg_core/executor/cluster_executor.py
@@ -38,9 +38,8 @@ class ClusterExecutor(AppLogger):
             sleep(30)
         return self._job_exit_code()
 
-    @classmethod
-    def _get_writer(cls, job_name, working_dir, walltime=None, cpus=1, mem=2, jobs=1, log_commands=True):
-        return cls.script_writer(job_name, working_dir, cfg['job_queue'], cpus, mem, walltime, jobs, log_commands)
+    def _get_writer(self, job_name, working_dir, walltime=None, cpus=1, mem=2, jobs=1, log_commands=True):
+        return self.script_writer(job_name, working_dir, self.job_queue, cpus, mem, walltime, jobs, log_commands)
 
     def _job_status(self):
         raise NotImplementedError
diff --git a/egcg_core/ncbi.py b/egcg_core/ncbi.py
index 69cc44c..13f74d6 100644
--- a/egcg_core/ncbi.py
+++ b/egcg_core/ncbi.py
@@ -76,7 +76,7 @@ def _fetch_from_eutils(species):
         rank = None
         if match:
             rank = match.group(1)
-        if rank in ['species', 'subspecies']:
+        if rank == 'species':
             scientific_name = common_name = None
             match = re.search('<ScientificName>(.+?)</ScientificName>', r.text, re.MULTILINE)
             if match:
diff --git a/egcg_core/rest_communication.py b/egcg_core/rest_communication.py
index 1b511aa..bf960cb 100644
--- a/egcg_core/rest_communication.py
+++ b/egcg_core/rest_communication.py
@@ -1,200 +1,182 @@
 import requests
 from urllib.parse import urljoin
-from base64 import b64encode
 from egcg_core.config import cfg
-from egcg_core.app_logging import AppLogger
+from egcg_core.app_logging import logging_default as log_cfg
 from egcg_core.exceptions import RestCommunicationError
 
+app_logger = log_cfg.get_logger(__name__)
+
+table = {' ': '', '\'': '"', 'None': 'null'}
+
+
+def _translate(s):
+    for k, v in table.items():
+        s = s.replace(k, v)
+    return s
+
+
+def api_url(endpoint, **query_args):
+    url = '{base_url}/{endpoint}/'.format(
+        base_url=cfg['rest_api']['url'].rstrip('/'), endpoint=endpoint
+    )
+    if query_args:
+        query = '?' + '&'.join(['%s=%s' % (k, v) for k, v in query_args.items()])
+        url += _translate(query)
+    return url
+
+
+def _parse_query_string(query_string, requires=None):
+    if '?' not in query_string:
+        return {}
+    if query_string.count('?') != 1:
+        raise RestCommunicationError('Bad query string: ' + query_string)
+    href, query = query_string.split('?')
+    query = dict([x.split('=') for x in query.split('&')])
+    if requires and not all([r in query for r in requires]):
+        raise RestCommunicationError('%s did not contain all required fields: %s' % (query_string, requires))
+    return query
+
+
+def _req(method, url, quiet=False, **kwargs):
+    auth = None
+    if 'username' in cfg['rest_api'] and 'password' in cfg['rest_api']:
+        auth = (cfg['rest_api']['username'], cfg['rest_api']['password'])
+
+    r = requests.request(method, url, auth=auth, **kwargs)
+    # e.g: 'POST <url> ({"some": "args"}) -> {"some": "content"}. Status code 201. Reason: CREATED
+    report = '%s %s (%s) -> %s. Status code %s. Reason: %s' % (
+        r.request.method, r.request.path_url, kwargs, r.content.decode('utf-8'), r.status_code, r.reason
+    )
+    if r.status_code in (200, 201):
+        if not quiet:
+            app_logger.debug(report)
+    elif r.status_code == 401:
+        raise RestCommunicationError('Invalid auth credentials')
+    else:
+        app_logger.error(report)
+    return r
+
+
+def get_content(endpoint, paginate=True, quiet=False, **query_args):
+    if paginate:
+        query_args.update(
+            max_results=query_args.pop('max_results', 100),  # default to page size of 100
+            page=query_args.pop('page', 1)
+        )
+    url = api_url(endpoint, **query_args)
+    return _req('GET', url, quiet=quiet).json()
 
-class Communicator(AppLogger):
-    table = {' ': '', '\'': '"', 'None': 'null'}
-    successful_statuses = (200, 201)
 
-    def __init__(self, auth=None, baseurl=None):
-        self._baseurl = baseurl
-        self._auth = auth
+def get_documents(endpoint, paginate=True, all_pages=False, quiet=False, **query_args):
+    content = get_content(endpoint, paginate, quiet, **query_args)
+    elements = content['data']
 
-    @property
-    def baseurl(self):
-        if self._baseurl is None:
-            self._baseurl = cfg['rest_api']['url'].rstrip('/')
-        return self._baseurl
+    if all_pages and 'next' in content['_links']:
+        next_query = _parse_query_string(content['_links']['next']['href'], requires=('max_results', 'page'))
+        query_args.update(next_query)
+        elements.extend(get_documents(endpoint, all_pages=True, quiet=quiet, **query_args))
 
-    @property
-    def auth(self):
-        if self._auth is None and 'username' in cfg['rest_api'] and 'password' in cfg['rest_api']:
-            self._auth = (cfg['rest_api']['username'], cfg['rest_api']['password'])
-        return self._auth
+    return elements
 
-    @staticmethod
-    def _hash_auth_token(token):
-        return b64encode(token.encode()).decode('utf-8')
 
-    @classmethod
-    def _translate(cls, s):
-        for k, v in cls.table.items():
-            s = s.replace(k, v)
-        return s
+def get_document(endpoint, idx=0, **query_args):
+    documents = get_documents(endpoint, **query_args)
+    if documents:
+        return documents[idx]
+    else:
+        app_logger.warning('No document found in endpoint %s for %s', endpoint, query_args)
 
-    def _api_url(self, endpoint, **query_args):
-        url = '{base_url}/{endpoint}/'.format(
-            base_url=self.baseurl, endpoint=endpoint
-        )
-        if query_args:
-            query = '?' + '&'.join(['%s=%s' % (k, v) for k, v in query_args.items()])
-            url += self._translate(query)
-        return url
-
-    @staticmethod
-    def _parse_query_string(query_string, requires=None):
-        if '?' not in query_string:
-            return {}
-        if query_string.count('?') != 1:
-            raise RestCommunicationError('Bad query string: ' + query_string)
-        href, query = query_string.split('?')
-        query = dict([x.split('=') for x in query.split('&')])
-        if requires and not all([r in query for r in requires]):
-            raise RestCommunicationError('%s did not contain all required fields: %s' % (query_string, requires))
-        return query
-
-    def _req(self, method, url, quiet=False, **kwargs):
-        if type(self.auth) is tuple:
-            kwargs.update(auth=self.auth)
-        elif type(self.auth) is str:
-            # noinspection PyTypeChecker
-            kwargs.update(headers={'Authorization': 'Token ' + self._hash_auth_token(self.auth)})
-
-        r = requests.request(method, url, **kwargs)
-        # e.g: 'POST <url> ({"some": "args"}) -> {"some": "content"}. Status code 201. Reason: CREATED
-        report = '%s %s (%s) -> %s. Status code %s. Reason: %s' % (
-            r.request.method, r.request.path_url, kwargs, r.content.decode('utf-8'), r.status_code, r.reason
-        )
-        if r.status_code in self.successful_statuses:
-            if not quiet:
-                self.debug(report)
-        elif r.status_code == 401:
-            raise RestCommunicationError('Invalid auth credentials')
-        else:
-            self.error(report)
-        return r
-
-    def get_content(self, endpoint, paginate=True, quiet=False, **query_args):
-        if paginate:
-            query_args.update(
-                max_results=query_args.pop('max_results', 100),  # default to page size of 100
-                page=query_args.pop('page', 1)
-            )
-        url = self._api_url(endpoint, **query_args)
-        return self._req('GET', url, quiet=quiet).json()
-
-    def get_documents(self, endpoint, paginate=True, all_pages=False, quiet=False, **query_args):
-        content = self.get_content(endpoint, paginate, quiet, **query_args)
-        elements = content['data']
-
-        if all_pages and 'next' in content['_links']:
-            next_query = self._parse_query_string(content['_links']['next']['href'], requires=('max_results', 'page'))
-            query_args.update(next_query)
-            elements.extend(self.get_documents(endpoint, all_pages=True, quiet=quiet, **query_args))
-
-        return elements
-
-    def get_document(self, endpoint, idx=0, **query_args):
-        documents = self.get_documents(endpoint, **query_args)
-        if documents:
-            return documents[idx]
-        else:
-            self.warning('No document found in endpoint %s for %s', endpoint, query_args)
-
-    def post_entry(self, endpoint, payload):
-        r = self._req('POST', self._api_url(endpoint), json=payload)
-        return r.status_code in self.successful_statuses
-
-    def put_entry(self, endpoint, element_id, payload):
-        r = self._req('PUT', urljoin(self._api_url(endpoint), element_id), json=payload)
-        return r.status_code in self.successful_statuses
-
-    def _patch_entry(self, endpoint, doc, payload, update_lists=None):
-        """
-        Patch a specific database item (specified by doc) with the given data payload.
-        :param str endpoint:
-        :param dict doc: The entry in the database to patch (contains the relevant _id and _etag)
-        :param dict payload: Data with which to patch doc
-        :param list update_lists: Doc items listed here will be appended rather than replaced by the patch
-        """
-        url = urljoin(self._api_url(endpoint), doc['_id'])
-        _payload = dict(payload)
-        headers = {'If-Match': doc.get('_etag')}
-        if update_lists:
-            for l in update_lists:
-                content = doc.get(l, [])
-                new_content = [x for x in _payload.get(l, []) if x not in content]
-                _payload[l] = content + new_content
-        r = self._req('PATCH', url, headers=headers, json=_payload)
-        return r.status_code in self.successful_statuses
-
-    def patch_entry(self, endpoint, payload, id_field, element_id, update_lists=None):
-        """
-        Retrieve a document at the given endpoint with the given unique ID, and patch it with some data.
-        :param str endpoint:
-        :param dict payload:
-        :param str id_field: The name of the unique identifier (e.g. 'run_element_id', 'proc_id', etc.)
-        :param element_id: The value of id_field to retrieve (e.g. '160301_2_ATGCATGC')
-        :param list update_lists:
-        """
-        doc = self.get_document(endpoint, where={id_field: element_id})
-        if doc:
-            return self._patch_entry(endpoint, doc, payload, update_lists)
-        return False
 
-    def patch_entries(self, endpoint, payload, update_lists=None, **query_args):
-        """
-        Retrieve many documents and patch them all with the same data.
-        :param str endpoint:
-        :param dict payload:
-        :param list update_lists:
-        :param query_args: Database query args to pass to get_documents
-        """
-        docs = self.get_documents(endpoint, **query_args)
-        if docs:
-            success = True
-            nb_docs = 0
-            for doc in docs:
-                if self._patch_entry(endpoint, doc, payload, update_lists):
-                    nb_docs += 1
-                else:
-                    success = False
-            self.info('Updated %s documents matching %s', nb_docs, query_args)
-            return success
+def post_entry(endpoint, payload):
+    r = _req('POST', api_url(endpoint), json=payload)
+    if r.status_code != 200:
         return False
+    return True
 
-    def post_or_patch(self, endpoint, input_json, id_field=None, update_lists=None):
-        """
-        For each document supplied, either post to the endpoint if the unique id doesn't yet exist there, or
-        patch if it does.
-        :param str endpoint:
-        :param input_json: A single document or list of documents to post or patch to the endpoint.
-        :param str id_field: The field to use as the unique ID for the endpoint.
-        :param list update_lists:
-        """
+
+def put_entry(endpoint, element_id, payload):
+    r = _req('PUT', urljoin(api_url(endpoint), element_id), json=payload)
+    if r.status_code != 200:
+        return False
+    return True
+
+
+def _patch_entry(endpoint, doc, payload, update_lists=None):
+    """
+    Patch a specific database item (specified by doc) with the given data payload.
+    :param str endpoint:
+    :param dict doc: The entry in the database to patch (contains the relevant _id and _etag)
+    :param dict payload: Data with which to patch doc
+    :param list update_lists: Doc items listed here will be appended rather than replaced by the patch
+    """
+    url = urljoin(api_url(endpoint), doc['_id'])
+    _payload = dict(payload)
+    headers = {'If-Match': doc.get('_etag')}
+    if update_lists:
+        for l in update_lists:
+            content = doc.get(l, [])
+            new_content = [x for x in _payload.get(l, []) if x not in content]
+            _payload[l] = content + new_content
+    r = _req('PATCH', url, headers=headers, json=_payload)
+    if r.status_code == 200:
+        return True
+    return False
+
+
+def patch_entry(endpoint, payload, id_field, element_id, update_lists=None):
+    """
+    Retrieve a document at the given endpoint with the given unique ID, and patch it with some data.
+    :param str endpoint:
+    :param dict payload:
+    :param str id_field: The name of the unique identifier (e.g. 'run_element_id', 'proc_id', etc.)
+    :param element_id: The value of id_field to retrieve (e.g. '160301_2_ATGCATGC')
+    :param list update_lists:
+    """
+    doc = get_document(endpoint, where={id_field: element_id})
+    if doc:
+        return _patch_entry(endpoint, doc, payload, update_lists)
+    return False
+
+
+def patch_entries(endpoint, payload, update_lists=None, **query_args):
+    """
+    Retrieve many documents and patch them all with the same data.
+    :param str endpoint:
+    :param dict payload:
+    :param list update_lists:
+    :param query_args: Database query args to pass to get_documents
+    """
+    docs = get_documents(endpoint, **query_args)
+    if docs:
         success = True
-        for payload in input_json:
-            _payload = dict(payload)
-            doc = self.get_document(endpoint, where={id_field: _payload[id_field]})
-            if doc:
-                _payload.pop(id_field)
-                s = self._patch_entry(endpoint, doc, _payload, update_lists)
+        nb_docs = 0
+        for doc in docs:
+            if _patch_entry(endpoint, doc, payload, update_lists):
+                nb_docs += 1
             else:
-                s = self.post_entry(endpoint, _payload)
-            success = success and s
+                success = False
+        app_logger.info('Updated %s documents matching %s', nb_docs, query_args)
         return success
-
-
-default = Communicator()
-get_content = default.get_content
-get_documents = default.get_documents
-get_document = default.get_document
-post_entry = default.post_entry
-put_entry = default.put_entry
-patch_entry = default.patch_entry
-patch_entries = default.patch_entries
-post_or_patch = default.post_or_patch
+    return False
+
+
+def post_or_patch(endpoint, input_json, id_field=None, update_lists=None):
+    """
+    For each document supplied, either post to the endpoint if the unique id doesn't yet exist there, or
+    patch if it does.
+    :param str endpoint:
+    :param input_json: A single document or list of documents to post or patch to the endpoint.
+    :param str id_field: The field to use as the unique ID for the endpoint.
+    :param list update_lists:
+    """
+    success = True
+    for payload in input_json:
+        _payload = dict(payload)
+        doc = get_document(endpoint, where={id_field: _payload[id_field]})
+        if doc:
+            _payload.pop(id_field)
+            s = _patch_entry(endpoint, doc, _payload, update_lists)
+        else:
+            s = post_entry(endpoint, _payload)
+        success = success and s
+    return success


Test patch:
diff --git a/tests/test_clarity.py b/tests/test_clarity.py
index b4c3684..6e963a2 100644
--- a/tests/test_clarity.py
+++ b/tests/test_clarity.py
@@ -104,10 +104,9 @@ def test_get_list_of_samples():
     exp_lims_sample_ids = ['this', 'that:01', 'other _L:01']
     calling_sample_ids = ['this', 'that_01', 'other__L_01']
     fake_list_samples = [[FakeEntity(n)] for n in exp_lims_sample_ids]
-    pbatch = patched_lims('get_batch')
     psamples = patched_lims('get_samples', side_effect=fake_list_samples)
 
-    with pbatch, psamples as mocked_get_samples:
+    with patched_lims('get_batch'), psamples as mocked_get_samples:
         samples = clarity.get_list_of_samples(calling_sample_ids)
         assert [s.name for s in samples] == exp_lims_sample_ids
         mocked_get_samples.assert_any_call(name=['this', 'that_01', 'other__L_01'])
@@ -115,6 +114,23 @@ def test_get_list_of_samples():
         mocked_get_samples.assert_any_call(name=['other _L:01'])
 
 
+def test_get_list_of_samples_broken():
+    exp_lims_sample_ids = ['this', 'that:01', 'other _L:01']
+    calling_sample_ids = ['this', 'that_01', 'other__L_01']
+    fake_list_samples = [[FakeEntity(n)] for n in exp_lims_sample_ids]
+    psamples = patched_lims('get_samples', side_effect=fake_list_samples)
+    log_msgs = []
+    pwarn = patched('app_logger.warning', new=log_msgs.append)
+
+    with patched_lims('get_batch'), psamples as mocked_get_samples, pwarn:
+        samples = clarity.get_list_of_samples(calling_sample_ids + ['sample_not_in_lims'])
+        assert [s.name for s in samples] == exp_lims_sample_ids
+        mocked_get_samples.assert_any_call(name=['this', 'that_01', 'other__L_01', 'sample_not_in_lims'])
+        mocked_get_samples.assert_any_call(name=['other__L:01', 'sample_not_in_lims', 'that:01'])
+        mocked_get_samples.assert_any_call(name=['other _L:01', 'sample_not_in_lims'])
+        assert log_msgs == ["Could not find ['sample_not_in_lims'] in Lims"]
+
+
 @patched_lims('get_samples', side_effect=[[], [], [None]])
 def test_get_samples(mocked_lims):
     assert clarity.get_samples('a_sample_name__L_01') == [None]
@@ -206,8 +222,7 @@ def test_get_output_containers_from_sample_and_step_name(mocked_get_sample, mock
 
 
 @patched_clarity('get_sample_names_from_plate', ['this', 'that', 'other'])
-@patched_clarity('get_sample',
-                 Mock(artifact=Mock(container=FakeEntity('a_container', type=FakeEntity('96 well plate')))))
+@patched_clarity('get_sample', Mock(artifact=Mock(container=FakeEntity('a_container', type=FakeEntity('96 well plate')))))
 def test_get_samples_arrived_with(mocked_get_sample, mocked_names_from_plate):
     assert clarity.get_samples_arrived_with('a_sample_name') == ['this', 'that', 'other']
     mocked_get_sample.assert_called_with('a_sample_name')
diff --git a/tests/test_ncbi.py b/tests/test_ncbi.py
index af10556..ef931bb 100644
--- a/tests/test_ncbi.py
+++ b/tests/test_ncbi.py
@@ -24,11 +24,6 @@ def test_fetch_from_eutils():
         <OtherNames><CommonName>a common name</CommonName></OtherNames>
         <Rank>species</Rank>
     '''
-    ncbi_fetch_data_sub_spe = '''
-        <ScientificName>Genus species</ScientificName>
-        <OtherNames><CommonName>a common name</CommonName></OtherNames>
-        <Rank>subspecies</Rank>
-    '''
 
     patched_get = patch(
         'egcg_core.ncbi.requests.get',
@@ -39,15 +34,7 @@ def test_fetch_from_eutils():
             FakeRestResponse(content=ncbi_fetch_data)
         )
     )
-    patched_get2 = patch(
-        'egcg_core.ncbi.requests.get',
-        side_effect=(
-            FakeRestResponse(content=ncbi_search_data),
-            FakeRestResponse(content=ncbi_fetch_data_sub_spe),
-            FakeRestResponse(content=ncbi_fetch_data_sub_spe),
-            FakeRestResponse(content=ncbi_fetch_data_sub_spe)
-        )
-    )
+
     with patched_get as mocked_get:
         obs = fetch_from_eutils('a_species')
         assert obs == ('1337', 'Genus species', 'a common name')
@@ -59,10 +46,6 @@ def test_fetch_from_eutils():
             'http://eutils.ncbi.nlm.nih.gov/entrez/eutils/efetch.fcgi',
             params={'db': 'Taxonomy', 'id': '1337'}
         )
-    with patched_get2:
-        obs = fetch_from_eutils('a_species')
-        assert obs == ('1337', 'Genus species', 'a common name')
-
 
 
 def test_cache():
diff --git a/tests/test_rest_communication.py b/tests/test_rest_communication.py
index 948c111..86b94b9 100644
--- a/tests/test_rest_communication.py
+++ b/tests/test_rest_communication.py
@@ -13,14 +13,12 @@ def rest_url(endpoint):
 
 
 def ppath(extension):
-    return 'egcg_core.rest_communication.Communicator.' + extension
+    return 'egcg_core.rest_communication.' + extension
 
 
 test_endpoint = 'an_endpoint'
 test_request_content = {'data': ['some', {'test': 'content'}]}
-test_patch_document = {
-    '_id': '1337', '_etag': 1234567, 'uid': 'a_unique_id', 'list_to_update': ['this', 'that', 'other']
-}
+
 patched_response = patch(
     'requests.request',
     return_value=FakeRestResponse(status_code=200, content=test_request_content)
@@ -40,173 +38,162 @@ def query_args_from_url(url):
     return json.loads(json.dumps(d))
 
 
-class TestRestCommunication(TestEGCG):
-    def setUp(self):
-        self.comm = rest_communication.Communicator()
+def test_api_url_query_strings():
+    assert rest_communication.api_url('an_endpoint') == rest_url('an_endpoint')
+    exp = '?where={"this":"that"}&embedded={"things":1}&aggregate=True&sort=-_created'
+    obs = rest_communication.api_url(
+        'an_endpoint',
+        where={'this': 'that'},
+        embedded={'things': 1},
+        aggregate=True,
+        sort='-_created'
+    ).replace(rest_url('an_endpoint'), '')
+    assert sorted(obs.lstrip('?').split('&')) == sorted(exp.lstrip('?').split('&'))
+
+
+def test_parse_query_string():
+    query_string = 'http://a_url?this=that&other={"another":"more"}'
+    no_query_string = 'http://a_url'
+    dodgy_query_string = 'http://a_url?this=that?other=another'
+
+    p = rest_communication._parse_query_string
+
+    assert p(query_string) == {'this': 'that', 'other': '{"another":"more"}'}
+    assert p(no_query_string) == {}
+
+    with pytest.raises(RestCommunicationError) as e:
+        p(dodgy_query_string)
+        assert str(e) == 'Bad query string: ' + dodgy_query_string
+
+    with pytest.raises(RestCommunicationError) as e2:
+        p(query_string, requires=['things'])
+        assert str(e2) == query_string + ' did not contain all required fields: ' + str(['things'])
+
+
+@patched_response
+def test_req(mocked_response):
+    json_content = ['some', {'test': 'json'}]
+
+    response = rest_communication._req('METHOD', rest_url(test_endpoint), json=json_content)
+    assert response.status_code == 200
+    assert json.loads(response.content.decode('utf-8')) == response.json() == test_request_content
+    mocked_response.assert_called_with('METHOD', rest_url(test_endpoint), auth=auth, json=json_content)
+
+
+def test_get_documents_depaginate():
+    docs = (
+        FakeRestResponse(content={'data': ['this', 'that'], '_links': {'next': {'href': 'an_endpoint?max_results=101&page=2'}}}),
+        FakeRestResponse(content={'data': ['other', 'another'], '_links': {'next': {'href': 'an_endpoint?max_results=101&page=3'}}}),
+        FakeRestResponse(content={'data': ['more', 'things'], '_links': {}})
+    )
+    patched_req = patch(ppath('_req'), side_effect=docs)
+    with patched_req as mocked_req:
+        assert rest_communication.get_documents('an_endpoint', all_pages=True, max_results=101) == [
+            'this', 'that', 'other', 'another', 'more', 'things'
+        ]
+        assert all([a[0][1].startswith(rest_url('an_endpoint')) for a in mocked_req.call_args_list])
+        assert [query_args_from_url(a[0][1]) for a in mocked_req.call_args_list] == [
+            {'page': '1', 'max_results': '101'},
+            {'page': '2', 'max_results': '101'},
+            {'page': '3', 'max_results': '101'}
+        ]
+
+
+@patched_response
+def test_test_content(mocked_response):
+    data = rest_communication.get_content(test_endpoint, max_results=100, where={'a_field': 'thing'})
+    assert data == test_request_content
+    assert mocked_response.call_args[0][1].startswith(rest_url(test_endpoint))
+    assert query_args_from_url(mocked_response.call_args[0][1]) == {
+        'max_results': '100', 'where': {'a_field': 'thing'}, 'page': '1'
+    }
+
+
+def test_get_documents():
+    with patched_response:
+        data = rest_communication.get_documents(test_endpoint, max_results=100, where={'a_field': 'thing'})
+        assert data == test_request_content['data']
 
-    def test_hash_auth_token(self):
-        assert self.comm._hash_auth_token('a_token') == 'YV90b2tlbg=='
 
-    def test_translate(self):
-        assert self.comm._translate("  '' None") == '""null'
+def test_get_document():
+    expected = test_request_content['data'][0]
+    with patched_response:
+        observed = rest_communication.get_document(test_endpoint, max_results=100, where={'a_field': 'thing'})
+        assert observed == expected
 
-    def test_api_url(self):
-        assert self.comm._api_url('an_endpoint') == rest_url('an_endpoint')
-        exp = '?where={"this":"that"}&embedded={"things":1}&aggregate=True&sort=-_created'
-        obs = self.comm._api_url(
+
+@patched_response
+def test_post_entry(mocked_response):
+    rest_communication.post_entry(test_endpoint, payload=test_request_content)
+    mocked_response.assert_called_with('POST', rest_url(test_endpoint), auth=auth, json=test_request_content)
+
+
+@patched_response
+def test_put_entry(mocked_response):
+    rest_communication.put_entry(test_endpoint, 'an_element_id', payload=test_request_content)
+    mocked_response.assert_called_with('PUT', rest_url(test_endpoint) + 'an_element_id', auth=auth, json=test_request_content)
+
+
+test_patch_document = {
+    '_id': '1337', '_etag': 1234567, 'uid': 'a_unique_id', 'list_to_update': ['this', 'that', 'other']
+}
+
+
+@patch('egcg_core.rest_communication.get_document', return_value=test_patch_document)
+@patched_response
+def test_patch_entry(mocked_response, mocked_get_doc):
+    patching_payload = {'list_to_update': ['another']}
+    rest_communication.patch_entry(
+        test_endpoint,
+        payload=patching_payload,
+        id_field='uid',
+        element_id='a_unique_id',
+        update_lists=['list_to_update']
+    )
+
+    mocked_get_doc.assert_called_with(test_endpoint, where={'uid': 'a_unique_id'})
+    mocked_response.assert_called_with(
+        'PATCH',
+        rest_url(test_endpoint) + '1337',
+        headers={'If-Match': 1234567},
+        auth=auth,
+        json={'list_to_update': ['this', 'that', 'other', 'another']}
+    )
+
+
+test_post_or_patch_payload = {'uid': '1337', 'list_to_update': ['more'], 'another_field': 'that'}
+test_post_or_patch_payload_no_uid = {'list_to_update': ['more'], 'another_field': 'that'}
+test_post_or_patch_doc = {
+    'uid': 'a_uid', '_id': '1337', '_etag': 1234567, 'list_to_update': ['things'], 'another_field': 'this'
+}
+
+
+def test_post_or_patch():
+    patched_post = patch(ppath('post_entry'), return_value=True)
+    patched_patch = patch(ppath('_patch_entry'), return_value=True)
+    patched_get = patch(ppath('get_document'), return_value=test_post_or_patch_doc)
+    patched_get_none = patch(ppath('get_document'), return_value=None)
+
+    with patched_get as mget, patched_patch as mpatch:
+        success = rest_communication.post_or_patch(
             'an_endpoint',
-            where={'this': 'that'},
-            embedded={'things': 1},
-            aggregate=True,
-            sort='-_created'
-        ).replace(rest_url('an_endpoint'), '')
-        assert sorted(obs.lstrip('?').split('&')) == sorted(exp.lstrip('?').split('&'))
-
-    def test_parse_query_string(self):
-        query_string = 'http://a_url?this=that&other={"another":"more"}'
-        no_query_string = 'http://a_url'
-        dodgy_query_string = 'http://a_url?this=that?other=another'
-
-        p = self.comm._parse_query_string
-
-        assert p(query_string) == {'this': 'that', 'other': '{"another":"more"}'}
-        assert p(no_query_string) == {}
-
-        with pytest.raises(RestCommunicationError) as e:
-            p(dodgy_query_string)
-            assert str(e) == 'Bad query string: ' + dodgy_query_string
-
-        with pytest.raises(RestCommunicationError) as e2:
-            p(query_string, requires=['things'])
-            assert str(e2) == query_string + ' did not contain all required fields: ' + str(['things'])
-
-    @patched_response
-    def test_req(self, mocked_response):
-        json_content = ['some', {'test': 'json'}]
-
-        response = self.comm._req('METHOD', rest_url(test_endpoint), json=json_content)
-        assert response.status_code == 200
-        assert json.loads(response.content.decode('utf-8')) == response.json() == test_request_content
-        mocked_response.assert_called_with('METHOD', rest_url(test_endpoint), auth=auth, json=json_content)
-
-    def test_get_documents_depaginate(self):
-        docs = (
-            FakeRestResponse(content={'data': ['this', 'that'], '_links': {'next': {'href': 'an_endpoint?max_results=101&page=2'}}}),
-            FakeRestResponse(content={'data': ['other', 'another'], '_links': {'next': {'href': 'an_endpoint?max_results=101&page=3'}}}),
-            FakeRestResponse(content={'data': ['more', 'things'], '_links': {}})
-        )
-        patched_req = patch(ppath('_req'), side_effect=docs)
-        with patched_req as mocked_req:
-            assert self.comm.get_documents('an_endpoint', all_pages=True, max_results=101) == [
-                'this', 'that', 'other', 'another', 'more', 'things'
-            ]
-            assert all([a[0][1].startswith(rest_url('an_endpoint')) for a in mocked_req.call_args_list])
-            assert [query_args_from_url(a[0][1]) for a in mocked_req.call_args_list] == [
-                {'page': '1', 'max_results': '101'},
-                {'page': '2', 'max_results': '101'},
-                {'page': '3', 'max_results': '101'}
-            ]
-
-    @patched_response
-    def test_get_content(self, mocked_response):
-        data = self.comm.get_content(test_endpoint, max_results=100, where={'a_field': 'thing'})
-        assert data == test_request_content
-        assert mocked_response.call_args[0][1].startswith(rest_url(test_endpoint))
-        assert query_args_from_url(mocked_response.call_args[0][1]) == {
-            'max_results': '100', 'where': {'a_field': 'thing'}, 'page': '1'
-        }
-
-    def test_get_documents(self):
-        with patched_response:
-            data = self.comm.get_documents(test_endpoint, max_results=100, where={'a_field': 'thing'})
-            assert data == test_request_content['data']
-
-    def test_get_document(self):
-        expected = test_request_content['data'][0]
-        with patched_response:
-            observed = self.comm.get_document(test_endpoint, max_results=100, where={'a_field': 'thing'})
-            assert observed == expected
-
-    @patched_response
-    def test_post_entry(self, mocked_response):
-        self.comm.post_entry(test_endpoint, payload=test_request_content)
-        mocked_response.assert_called_with('POST', rest_url(test_endpoint), auth=auth, json=test_request_content)
-
-    @patched_response
-    def test_put_entry(self, mocked_response):
-        self.comm.put_entry(test_endpoint, 'an_element_id', payload=test_request_content)
-        mocked_response.assert_called_with('PUT', rest_url(test_endpoint) + 'an_element_id', auth=auth, json=test_request_content)
-
-    @patch(ppath('get_document'), return_value=test_patch_document)
-    @patched_response
-    def test_patch_entry(self, mocked_response, mocked_get_doc):
-        patching_payload = {'list_to_update': ['another']}
-        self.comm.patch_entry(
-            test_endpoint,
-            payload=patching_payload,
+            [test_post_or_patch_payload],
             id_field='uid',
-            element_id='a_unique_id',
             update_lists=['list_to_update']
         )
-
-        mocked_get_doc.assert_called_with(test_endpoint, where={'uid': 'a_unique_id'})
-        mocked_response.assert_called_with(
-            'PATCH',
-            rest_url(test_endpoint) + '1337',
-            headers={'If-Match': 1234567},
-            auth=auth,
-            json={'list_to_update': ['this', 'that', 'other', 'another']}
+        mget.assert_called_with('an_endpoint', where={'uid': '1337'})
+        mpatch.assert_called_with(
+            'an_endpoint',
+            test_post_or_patch_doc,
+            test_post_or_patch_payload_no_uid,
+            ['list_to_update']
         )
+        assert success is True
 
-    def test_post_or_patch(self):
-        test_post_or_patch_payload = {'uid': '1337', 'list_to_update': ['more'], 'another_field': 'that'}
-        test_post_or_patch_payload_no_uid = {'list_to_update': ['more'], 'another_field': 'that'}
-        test_post_or_patch_doc = {
-            'uid': 'a_uid', '_id': '1337', '_etag': 1234567, 'list_to_update': ['things'], 'another_field': 'this'
-        }
-        patched_post = patch(ppath('post_entry'), return_value=True)
-        patched_patch = patch(ppath('_patch_entry'), return_value=True)
-        patched_get = patch(ppath('get_document'), return_value=test_post_or_patch_doc)
-        patched_get_none = patch(ppath('get_document'), return_value=None)
-
-        with patched_get as mget, patched_patch as mpatch:
-            success = self.comm.post_or_patch(
-                'an_endpoint',
-                [test_post_or_patch_payload],
-                id_field='uid',
-                update_lists=['list_to_update']
-            )
-            mget.assert_called_with('an_endpoint', where={'uid': '1337'})
-            mpatch.assert_called_with(
-                'an_endpoint',
-                test_post_or_patch_doc,
-                test_post_or_patch_payload_no_uid,
-                ['list_to_update']
-            )
-            assert success is True
-
-        with patched_get_none as mget, patched_post as mpost:
-            success = self.comm.post_or_patch(
-                'an_endpoint', [test_post_or_patch_payload], id_field='uid', update_lists=['list_to_update']
-            )
-            mget.assert_called_with('an_endpoint', where={'uid': '1337'})
-            mpost.assert_called_with('an_endpoint', test_post_or_patch_payload)
-            assert success is True
-
-    def test_token_auth(self):
-        token = '{"token": "some"}.tokenauthentication'
-        hashed_token = 'eyJ0b2tlbiI6ICJzb21lIn0udG9rZW5hdXRoZW50aWNhdGlvbg=='
-        self.comm._auth = token
-        with patched_response as p:
-            self.comm._req('GET', self.comm.baseurl + 'an_endpoint')
-            p.assert_called_with(
-                'GET',
-                self.comm.baseurl + 'an_endpoint',
-                headers={'Authorization': 'Token ' + hashed_token}
-            )
-
-
-def test_default():
-    d = rest_communication.default
-    assert d.baseurl == 'http://localhost:4999/api/0.1'
-    assert d.auth == ('a_user', 'a_password')
+    with patched_get_none as mget, patched_post as mpost:
+        success = rest_communication.post_or_patch(
+            'an_endpoint', [test_post_or_patch_payload], id_field='uid', update_lists=['list_to_update']
+        )
+        mget.assert_called_with('an_endpoint', where={'uid': '1337'})
+        mpost.assert_called_with('an_endpoint', test_post_or_patch_payload)
+        assert success is True