From dbbff3ba64d5dca7745b29320ade0b2156c63253 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 28 May 2020 13:22:47 +0100 Subject: [PATCH 1/2] =?UTF-8?q?Add=20property=20to=20contact=20lists=20to?= =?UTF-8?q?=20say=20if=20they=E2=80=99ve=20ever=20been=20used?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit At the moment we return a count of recent jobs for contact lists, where recent is defined as being within the service’s data retention period. This lets us write nice bits of UI copy like ‘used 3 times in the last 7 days’. But it’s hard to write the copy for when the count is 0, because this could be for one of two reasons: - the contact list has never been used - the contact list has been used, but not within the data retention period for that channel At the moment we can’t know which of those reasons is the case, so we can’t write nice clear content like ‘never been used’. This commit adds a property to contact lists which says whether they’ve ever been used. It also renames the existing, as-yet-unused property to make clear that it’s only counting within the data retention (so can still be 0 even if `has_jobs` is `True`). --- app/models.py | 8 +++++++- tests/app/service/test_service_contact_list_rest.py | 8 +++++--- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/app/models.py b/app/models.py index 2f4d7a5bb..d09f18e7b 100644 --- a/app/models.py +++ b/app/models.py @@ -2136,13 +2136,19 @@ class ServiceContactList(db.Model): ) ).count() + def get_has_jobs(self): + return bool(Job.query.filter( + Job.contact_list_id == self.id, + ).first()) + def serialize(self): created_at_in_bst = convert_utc_to_bst(self.created_at) contact_list = { "id": str(self.id), "original_file_name": self.original_file_name, "row_count": self.row_count, - "job_count": self.get_job_count(), + "recent_job_count": self.get_job_count(), + "has_jobs": self.get_has_jobs(), "template_type": self.template_type, "service_id": str(self.service_id), "created_by": self.created_by.name, diff --git a/tests/app/service/test_service_contact_list_rest.py b/tests/app/service/test_service_contact_list_rest.py index 3ce45755d..82a1fc1cb 100644 --- a/tests/app/service/test_service_contact_list_rest.py +++ b/tests/app/service/test_service_contact_list_rest.py @@ -69,7 +69,7 @@ def test_get_contact_list(admin_request, notify_db_session): assert len(response) == 1 assert response[0] == contact_list.serialize() - assert response[0]['job_count'] == 0 + assert response[0]['recent_job_count'] == 0 @pytest.mark.parametrize('days_of_email_retention, expected_job_count', ( @@ -107,10 +107,12 @@ def test_get_contact_list_counts_jobs( assert len(response) == 2 assert response[0]['id'] == str(contact_list_2.id) - assert response[0]['job_count'] == expected_job_count + assert response[0]['recent_job_count'] == expected_job_count + assert response[0]['has_jobs'] is True assert response[1]['id'] == str(contact_list_1.id) - assert response[1]['job_count'] == 0 + assert response[1]['recent_job_count'] == 0 + assert response[1]['has_jobs'] is False def test_get_contact_list_returns_for_service(admin_request, notify_db_session): From 313b69f95c6c963650d478a1281729cab5935e11 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 21 Jul 2020 15:12:44 +0100 Subject: [PATCH 2/2] Make property methods into proper properties MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since these methods only take `self` as an argument they can be properties. And this means we don’t need to follow the semi-convention we have of indicating an attribute is a method by starting its name with `get_`. The other advantage of using `@property` to indicate a getter is that it will raise an exception if someone tries to set the attribute, eg by doing `contact_list.has_jobs = False`. This is because we (rightly) haven’t defined a setter. --- app/models.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/app/models.py b/app/models.py index d09f18e7b..9c5450fb5 100644 --- a/app/models.py +++ b/app/models.py @@ -2122,7 +2122,8 @@ class ServiceContactList(db.Model): updated_at = db.Column(db.DateTime, nullable=True, onupdate=datetime.datetime.utcnow) archived = db.Column(db.Boolean, nullable=False, default=False) - def get_job_count(self): + @property + def job_count(self): today = datetime.datetime.utcnow().date() return Job.query.filter( Job.contact_list_id == self.id, @@ -2136,7 +2137,8 @@ class ServiceContactList(db.Model): ) ).count() - def get_has_jobs(self): + @property + def has_jobs(self): return bool(Job.query.filter( Job.contact_list_id == self.id, ).first()) @@ -2147,8 +2149,8 @@ class ServiceContactList(db.Model): "id": str(self.id), "original_file_name": self.original_file_name, "row_count": self.row_count, - "recent_job_count": self.get_job_count(), - "has_jobs": self.get_has_jobs(), + "recent_job_count": self.job_count, + "has_jobs": self.has_jobs, "template_type": self.template_type, "service_id": str(self.service_id), "created_by": self.created_by.name,