Repository: saleor/saleor
Pull request: Add publication date to collections
I want to merge this change because...

resolves #3125 

1. [x] Privileged views and APIs are guarded by proper permission checks.
1. [x] All visible strings are translated with proper context.
1. [x] All data-formatting is locale-aware (dates, numbers, and so on).
1. [x] Database queries are optimized and the number of queries is constant.
1. [x] The changes are tested.
1. [x] The code is documented (docstrings, project documentation).
1. [x] GraphQL schema and type definitions are up to date.


Problem statement:
Add publication date to collections
### What I'm trying to achieve
To publish collection with some delay.

### Describe a proposed solution
Add `published_at`field to collection model and check if the current date is greater than `published_at`. Basically the same as `available_on` in products.

Hints from issue discussion:
Can I take this up?
@lokesh005 go for it, there's similar mechanism already live on `Page` model, you might want to take some inspiration out of it 

Any questions- I'm here to help

Commit message: Review fixes - renaming of published_at field and available property, changes in collections logic in templates

Patch to review:
diff --git a/saleor/core/templatetags/status.py b/saleor/core/templatetags/status.py
index 24f87ad1ee0..642036802c1 100644
--- a/saleor/core/templatetags/status.py
+++ b/saleor/core/templatetags/status.py
@@ -68,10 +68,10 @@ def render_page_availability(page):
 
 @register.inclusion_tag('dashboard/includes/_collection_availability.html')
 def render_collection_availability(collection):
-    if collection.is_published:
+    if collection.is_visible:
         label_cls = LABEL_SUCCESS
     else:
         label_cls = LABEL_DANGER
-    return {'is_published': collection.is_published,
+    return {'is_visible': collection.is_visible,
             'collection': collection,
             'label_cls': label_cls}
diff --git a/saleor/dashboard/collection/forms.py b/saleor/dashboard/collection/forms.py
index 67dd0893bd5..cd5d1f430a0 100644
--- a/saleor/dashboard/collection/forms.py
+++ b/saleor/dashboard/collection/forms.py
@@ -28,6 +28,9 @@ class Meta:
             'is_published': pgettext_lazy(
                 'Collection published toggle',
                 'Published'),
+            'published_date': pgettext_lazy(
+                'The publication date field, can be a posterior date for a planned publication.',
+                'Published date'),
             'description': pgettext_lazy(
                 'Description field of a collection',
                 'Description')}
diff --git a/saleor/graphql/product/mutations/products.py b/saleor/graphql/product/mutations/products.py
index 316232259b9..362fcd3a56a 100644
--- a/saleor/graphql/product/mutations/products.py
+++ b/saleor/graphql/product/mutations/products.py
@@ -110,6 +110,8 @@ class CollectionInput(graphene.InputObjectType):
     description = graphene.String(description='Description of the collection.')
     background_image = Upload(description='Background image file.')
     seo = SeoInput(description='Search engine optimization fields.')
+    published_date = graphene.String(
+        description='Publication date. ISO 8601 standard.')
 
 
 class CollectionCreateInput(CollectionInput):
diff --git a/saleor/graphql/schema.graphql b/saleor/graphql/schema.graphql
index e428f9a1f7d..2444b63ad0a 100644
--- a/saleor/graphql/schema.graphql
+++ b/saleor/graphql/schema.graphql
@@ -408,6 +408,7 @@ type Collection implements Node {
   backgroundImage: Image
   isPublished: Boolean!
   description: String!
+  publishedDate: Date
 }
 
 type CollectionAddProducts {
@@ -438,6 +439,7 @@ input CollectionCreateInput {
   description: String
   backgroundImage: Upload
   seo: SeoInput
+  publishedDate: String
   products: [ID]
 }
 
@@ -453,6 +455,7 @@ input CollectionInput {
   description: String
   backgroundImage: Upload
   seo: SeoInput
+  publishedDate: String
 }
 
 type CollectionRemoveProducts {
@@ -668,6 +671,7 @@ input FulfillmentUpdateTrackingInput {
 enum GatewaysEnum {
   DUMMY
   BRAINTREE
+  RAZORPAY
 }
 
 scalar GenericScalar
diff --git a/saleor/product/migrations/0080_collection_published_at.py b/saleor/product/migrations/0080_collection_published_at.py
new file mode 100644
index 00000000000..0e57d501bcb
--- /dev/null
+++ b/saleor/product/migrations/0080_collection_published_at.py
@@ -0,0 +1,18 @@
+# Generated by Django 2.1.3 on 2018-11-30 21:40
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('product', '0079_default_tax_rate_instead_of_empty_field'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='collection',
+            name='published_at',
+            field=models.DateField(blank=True, null=True),
+        ),
+    ]
diff --git a/saleor/product/migrations/0081_auto_20181202_1337.py b/saleor/product/migrations/0081_auto_20181202_1337.py
new file mode 100644
index 00000000000..8d5d220beb9
--- /dev/null
+++ b/saleor/product/migrations/0081_auto_20181202_1337.py
@@ -0,0 +1,18 @@
+# Generated by Django 2.1.3 on 2018-12-02 19:37
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('product', '0080_collection_published_at'),
+    ]
+
+    operations = [
+        migrations.RenameField(
+            model_name='collection',
+            old_name='published_at',
+            new_name='published_date',
+        ),
+    ]
diff --git a/saleor/product/models.py b/saleor/product/models.py
index 5391cbd34db..dad1a3bbeb1 100644
--- a/saleor/product/models.py
+++ b/saleor/product/models.py
@@ -435,7 +435,9 @@ class VariantImage(models.Model):
 class CollectionQuerySet(models.QuerySet):
 
     def public(self):
-        return self.filter(is_published=True)
+        return self.filter(
+            Q(is_published=True),
+            Q(published_date__lte=datetime.date.today()) | Q(published_date__isnull=True))
 
     def visible_to_user(self, user):
         has_access_to_all = (
@@ -454,7 +456,7 @@ class Collection(SeoModel):
         upload_to='collection-backgrounds', blank=True, null=True)
     is_published = models.BooleanField(default=False)
     description = models.TextField(blank=True)
-
+    published_date = models.DateField(blank=True, null=True)
     objects = CollectionQuerySet.as_manager()
     translated = TranslationProxy()
 
@@ -469,6 +471,11 @@ def get_absolute_url(self):
             'product:collection',
             kwargs={'pk': self.id, 'slug': self.slug})
 
+    @property
+    def is_visible(self):
+        return self.is_published and (
+            self.published_date is None or self.published_date <= datetime.date.today())
+
 
 class CollectionTranslation(SeoModelTranslation):
     language_code = models.CharField(max_length=10)
diff --git a/templates/collection/index.html b/templates/collection/index.html
index 2bf55a91909..deff267d799 100644
--- a/templates/collection/index.html
+++ b/templates/collection/index.html
@@ -17,6 +17,13 @@
         You are previewing a collection that is not published.
       {% endblocktrans %}
     </div>
+  {% elif object.is_published and object.published_date is not None %}
+    <div class="alert alert-warning" role="alert">
+      {% blocktrans trimmed with date=object.published_date|date context "Unpublished collection details text" %}
+        <strong>Warning!</strong>
+        You are previewing a collection that will become visible on <strong>{{ date }}</strong>.
+      {% endblocktrans %}
+    </div>
   {% endif %}
 {% endblock %}
 
diff --git a/templates/dashboard/collection/detail.html b/templates/dashboard/collection/detail.html
index 771afcb1a84..939fd7899c9 100644
--- a/templates/dashboard/collection/detail.html
+++ b/templates/dashboard/collection/detail.html
@@ -64,6 +64,9 @@
             <div class="row">
               {{ form.is_published|materializecss }}
             </div>
+            <div class="row">
+              {{ form.published_date|materializecss }}
+            </div>
             {% include "dashboard/includes/_google_preview.html" with object=collection form=form %}
           </div>
           <div class="card-action right-align">
diff --git a/templates/dashboard/includes/_collection_availability.html b/templates/dashboard/includes/_collection_availability.html
index 780bffc2047..ae453d82fb2 100644
--- a/templates/dashboard/includes/_collection_availability.html
+++ b/templates/dashboard/includes/_collection_availability.html
@@ -1,9 +1,16 @@
 {% load i18n %}
 
   <div class="label label-{{ label_cls }}">
-    {% if is_published %}
+    {% if is_visible %}
       {% trans "Published" context "Collection availability status" %}
-    {% else %}
-      {% trans "Not Published" context "Collection availability status" %}
-    {% endif %}
+    {% elif not collection.is_published %}
+    <div class="label">
+      {% trans "Hidden" context "Collection availability status" %}
+    </div>
+    {% elif collection.is_published and collection.published_date is not None %}
+  <div class="label">
+    {% blocktrans with date=collection.published_date context "Collection availability status" %}
+      Hidden (will become visible on {{ date }})
+    {% endblocktrans %}
   </div>
+{% endif %}
diff --git a/tests/api/test_collection.py b/tests/api/test_collection.py
index 365320678e0..88ec3e71f9e 100644
--- a/tests/api/test_collection.py
+++ b/tests/api/test_collection.py
@@ -1,5 +1,6 @@
 import graphene
 import pytest
+from datetime import date
 from django.utils.text import slugify
 from graphql_relay import to_global_id
 from unittest.mock import Mock
@@ -41,7 +42,6 @@ def test_collections_query(
     assert collection_data['slug'] == collection.slug
     assert collection_data['description'] == collection.description
     assert collection_data['products']['totalCount'] == collection.products.count()
-
     # query all collections only as a staff user with proper permissions
     staff_api_client.user.user_permissions.add(permission_manage_products)
     response = staff_api_client.post_graphql(query)
@@ -54,9 +54,9 @@ def test_create_collection(
         monkeypatch, staff_api_client, product_list, permission_manage_products):
     query = """
         mutation createCollection(
-            $name: String!, $slug: String!, $description: String, $products: [ID], $backgroundImage: Upload!, $isPublished: Boolean!) {
+            $name: String!, $slug: String!, $description: String, $products: [ID], $backgroundImage: Upload!, $isPublished: Boolean!, $publishedDate: String) {
             collectionCreate(
-                input: {name: $name, slug: $slug, description: $description, products: $products, backgroundImage: $backgroundImage, isPublished: $isPublished}) {
+                input: {name: $name, slug: $slug, description: $description, products: $products, backgroundImage: $backgroundImage, isPublished: $isPublished, publishedDate: $publishedDate}) {
                 collection {
                     name
                     slug
@@ -64,6 +64,11 @@ def test_create_collection(
                     products {
                         totalCount
                     }
+                    publishedDate
+                }
+                errors {
+                    field
+                    message
                 }
             }
         }
@@ -81,10 +86,11 @@ def test_create_collection(
     name = 'test-name'
     slug = 'test-slug'
     description = 'test-description'
+    published_date = date.today().isoformat()
     variables = {
         'name': name, 'slug': slug, 'description': description,
         'products': product_ids, 'backgroundImage': image_name,
-        'isPublished': True}
+        'isPublished': True, 'publishedDate': published_date}
     body = get_multipart_request_body(query, variables, image_file, image_name)
     response = staff_api_client.post_multipart(
         body, permissions=[permission_manage_products])
@@ -93,6 +99,7 @@ def test_create_collection(
     assert data['name'] == name
     assert data['slug'] == slug
     assert data['description'] == description
+    assert data['publishedDate'] == published_date
     assert data['products']['totalCount'] == len(product_ids)
     collection = Collection.objects.get(slug=slug)
     assert collection.background_image.file
@@ -132,13 +139,14 @@ def test_update_collection(
         monkeypatch, staff_api_client, collection, permission_manage_products):
     query = """
         mutation updateCollection(
-            $name: String!, $slug: String!, $description: String, $id: ID!, $isPublished: Boolean!) {
+            $name: String!, $slug: String!, $description: String, $id: ID!, $isPublished: Boolean!, $publishedDate: String) {
             collectionUpdate(
-                id: $id, input: {name: $name, slug: $slug, description: $description, isPublished: $isPublished}) {
+                id: $id, input: {name: $name, slug: $slug, description: $description, isPublished: $isPublished, publishedDate: $publishedDate}) {
                 collection {
                     name
                     slug
                     description
+                    publishedDate
                 }
             }
         }
@@ -153,15 +161,17 @@ def test_update_collection(
     name = 'new-name'
     slug = 'new-slug'
     description = 'new-description'
+    published_date = date.today().isoformat()
     variables = {
         'name': name, 'slug': slug, 'description': description,
-        'id': to_global_id('Collection', collection.id), 'isPublished': True}
+        'id': to_global_id('Collection', collection.id), 'isPublished': True, 'publishedDate': published_date}
     response = staff_api_client.post_graphql(
         query, variables, permissions=[permission_manage_products])
     content = get_graphql_content(response)
     data = content['data']['collectionUpdate']['collection']
     assert data['name'] == name
     assert data['slug'] == slug
+    assert data['publishedDate'] == published_date
     assert mock_create_thumbnails.call_count == 0
 
 
diff --git a/tests/conftest.py b/tests/conftest.py
index 491ed17b250..45f14cdec58 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -1,6 +1,5 @@
 from io import BytesIO
 from unittest.mock import MagicMock, Mock
-
 import pytest
 
 from django.contrib.auth.models import Permission
diff --git a/tests/test_collection.py b/tests/test_collection.py
index 0c1afb9b41a..5b2b5f6fa95 100644
--- a/tests/test_collection.py
+++ b/tests/test_collection.py
@@ -1,5 +1,5 @@
 from django.urls import reverse
-
+from datetime import date, timedelta
 from .utils import get_redirect_location
 
 
@@ -33,8 +33,17 @@ def test_collection_not_exists(client):
     assert response.status_code == 404
 
 
-def test_collection_not_published_404(client, draft_collection):
+def test_collection_not_published_404(admin_client, client, draft_collection):
     url_kwargs = {'pk': draft_collection.pk, 'slug': draft_collection.slug}
     url = reverse('product:collection', kwargs=url_kwargs)
     response = client.get(url)
     assert response.status_code == 404
+
+    draft_collection.is_published = True
+    draft_collection.published_date = date.today() + timedelta(days=1)
+    draft_collection.save()
+    response = client.get(url)
+    assert response.status_code == 404
+
+    response = admin_client.get(url)
+    assert response.status_code == 200


Code review comments:
diff_hunk: @@ -435,10 +435,9 @@ class VariantImage(models.Model):
 class CollectionQuerySet(models.QuerySet):
 
     def public(self):
-        today = datetime.datetime.today()
         return self.filter(
             Q(is_published=True),
-            Q(published_at__lte=today) | Q(published_at__isnull=True))
+            Q(published_date__lte=datetime.date.today()) | Q(published_date__isnull=True))
path: saleor/product/models.py
text: @user1:
Wouldn't be switching the two conditions be faster?

```diff
-published_date <= today or published_date is None
+published_date is None or published_date <= today
```

diff_hunk: @@ -17,9 +17,9 @@
         You are previewing a collection that is not published.
       {% endblocktrans %}
     </div>
-  {% elif not is_available %}
+  {% elif object.is_published and object.published_date is not None %}
path: templates/collection/index.html
text: @user1:
We already know the collection is published, no need to evaluate twice.

diff_hunk: @@ -1,20 +1,16 @@
 {% load i18n %}
 
   <div class="label label-{{ label_cls }}">
-    {% if is_available %}
+    {% if is_visible %}
       {% trans "Published" context "Collection availability status" %}
     {% elif not collection.is_published %}
     <div class="label">
       {% trans "Hidden" context "Collection availability status" %}
     </div>
-    {% elif collection.published_at %}
+    {% elif collection.is_published and collection.published_date is not None %}
path: templates/dashboard/includes/_collection_availability.html
text: @user1:
`collection.is_published` is `True`, no need to check twice.

The logic is still wrong though, you want to check `date is not None` first, then have a `else` block.