CB-390: Shouldn't have to choose a license just to save drafts

admin/schema_changes/24.sql

@@ -0,0 +1,17 @@
+BEGIN;
+
+ALTER TABLE review ADD CONSTRAINT license_id_not_null_is_draft_false
+    CHECK ((is_draft = 'f' AND license_id IS NOT NULL) OR is_draft = 't');
+
+ALTER TABLE review ALTER COLUMN license_id DROP NOT NULL;
+
+-- to change the ON DELETE action of foreign key need to drop and recreate it
+ALTER TABLE review DROP CONSTRAINT review_license_id_fkey;
+
+ALTER TABLE review
+  ADD CONSTRAINT review_license_id_fkey
+  FOREIGN KEY (license_id)
+  REFERENCES license(id)
+  ON DELETE SET NULL;
+
+COMMIT;

admin/sql/create_foreign_keys.sql

@@ -4,7 +4,7 @@ ALTER TABLE review
   ADD CONSTRAINT review_license_id_fkey
   FOREIGN KEY (license_id)
   REFERENCES license(id)
-  ON DELETE CASCADE;
+  ON DELETE SET NULL;
 
 ALTER TABLE moderation_log
   ADD CONSTRAINT moderation_log_admin_id_fkey

admin/sql/create_tables.sql

@@ -72,7 +72,7 @@ CREATE TABLE review (
     edits           INTEGER      NOT NULL,
     is_draft        BOOLEAN      NOT NULL,
     is_hidden       BOOLEAN      NOT NULL,
-    license_id      VARCHAR      NOT NULL,
+    license_id      VARCHAR,
     language        VARCHAR(3)   NOT NULL,
     published_on    TIMESTAMP,
     source          VARCHAR,
@@ -81,6 +81,8 @@ CREATE TABLE review (
 ALTER TABLE review ADD CONSTRAINT review_entity_id_user_id_key UNIQUE (entity_id, user_id);
 ALTER TABLE review ADD CONSTRAINT published_on_null_for_drafts_and_not_null_for_published_reviews
     CHECK ((is_draft = 't' AND published_on IS NULL) OR (is_draft = 'f' And published_on IS NOT NULL));
+ALTER TABLE review ADD CONSTRAINT license_id_not_null_is_draft_false
+    CHECK ((is_draft = 'f' AND license_id IS NOT NULL) OR is_draft = 't');
 
 CREATE TABLE revision (
     id          SERIAL      NOT NULL,

critiquebrainz/db/__init__.py

@@ -3,7 +3,7 @@
 from sqlalchemy.pool import NullPool
 
 # This value must be incremented after schema changes on exported tables!
-SCHEMA_VERSION = 15
+SCHEMA_VERSION = 16
 
 VALID_RATING_VALUES = [None, 1, 2, 3, 4, 5]
 REVIEW_RATING_MAX = 5

critiquebrainz/db/review.py

@@ -158,7 +158,7 @@ def get_by_ids(review_ids: List[uuid.UUID]):
               FROM review
               JOIN revision ON revision.review_id = review.id
               JOIN "user" ON "user".id = review.user_id
-              JOIN license ON license.id = license_id
+         LEFT JOIN license ON license.id = license_id
              WHERE review.id IN :review_ids
           ORDER BY timestamp DESC
         """), {
@@ -551,7 +551,7 @@ def get_reviews_list(connection, *, inc_drafts=False, inc_hidden=False, entity_i
           JOIN revision ON review.id = revision.review_id
      LEFT JOIN vote ON vote.revision_id = revision.id
           JOIN "user" ON review.user_id = "user".id
-          JOIN license ON license.id = license_id
+     LEFT JOIN license ON license.id = license_id
         {latest_revision_query}   
         {where_clause}
       GROUP BY review.id, latest_revision.id, "user".id, license.id

critiquebrainz/frontend/forms/review.py

@@ -32,12 +32,14 @@ class ReviewEditForm(FlaskForm):
         StateAndLength(min=MIN_REVIEW_LENGTH, max=MAX_REVIEW_LENGTH,
                        message=lazy_gettext("Text length needs to be between %(min)d and %(max)d characters.",
                                             min=MIN_REVIEW_LENGTH, max=MAX_REVIEW_LENGTH))])
+
     license_choice = RadioField(
         choices=[
             ('CC BY-SA 3.0', lazy_gettext('Allow commercial use of this review(<a href="https://creativecommons.org/licenses/by-sa/3.0/" target="_blank">CC BY-SA 3.0 license</a>)')),  # noqa: E501
             ('CC BY-NC-SA 3.0', lazy_gettext('Do not allow commercial use of this review, unless approved by MetaBrainz Foundation (<a href="https://creativecommons.org/licenses/by-nc-sa/3.0/" target="_blank">CC BY-NC-SA 3.0 license</a>)')),  # noqa: E501
         ],
-        validators=[validators.InputRequired(message=lazy_gettext("You need to choose a license"))])
+        validate_choice=False
+    )
     remember_license = BooleanField(lazy_gettext("Remember this license choice for further preference"))
     language = SelectField(lazy_gettext("You need to accept the license agreement!"), choices=languages)
     rating = IntegerField(lazy_gettext("Rating"), widget=Input(input_type='number'), validators=[validators.Optional()])
@@ -55,6 +57,27 @@ def validate(self):
             return False
         return True
 
+    def validate_license_choice(self, field):
+        if self.state.data == "draft":
+            return
+        else:
+            # https://wtforms.readthedocs.io/en/2.3.x/_modules/wtforms/validators/#InputRequired
+            if not field.raw_data or not field.raw_data[0]:
+                raise ValidationError(lazy_gettext("You need to choose a license."))
+
+            # choice validation for RadioField's is done in pre_validate in WTForms which executes prior to
+            # this validator. it does not take into consideration whether the review is draft or not so we
+            # have to disable it with validate_choice = False to allow no choice in license field. so we also
+            # have to manually validate the license choice here
+            match = False
+            for value, label in field.choices:
+                if value == field.data:
+                    match = True
+                    break
+
+            if not match:
+                raise ValidationError(lazy_gettext("Not a valid license choice."))
+
 
 class ReviewCreateForm(ReviewEditForm):
     agreement = BooleanField(validators=[

critiquebrainz/frontend/views/test/test_review.py

@@ -145,6 +145,73 @@ def test_create_duplicate(self):
                                    follow_redirects=True)
         self.assertIn("You have already published a review for this entity", str(response.data))
 
+    def test_draft_without_license_choice(self):
+        review = db_review.create(
+            entity_id="0cef1de0-6ff1-38e1-80cb-ff11ee2c69e2",
+            entity_type="release_group",
+            user_id=self.user.id,
+            text=self.review_text,
+            rating=self.review_rating,
+            is_draft=True,
+            license_id=None,
+        )
+
+        data = dict(
+            entity_id='0cef1de0-6ff1-38e1-80cb-ff11ee2c69e2',
+            entity_type='release_group',
+            state='draft',
+            text=self.review_text,
+            language='en',
+            agreement='True',
+            rating=5,
+        )
+        self.temporary_login(self.user)
+
+        # test draft review without license choice works
+        response = self.client.post(
+            "/review/%s/edit" % review["id"],
+            data=data,
+            query_string=data,
+            follow_redirects=True
+        )
+        self.assert200(response)
+        self.assertIn(self.review_text, str(response.data))
+
+        # test publishing draft review without license causes error
+        data["state"] = "publish"
+        response = self.client.post(
+            "/review/%s/edit" % review["id"],
+            data=data,
+            query_string=data,
+            follow_redirects=True
+        )
+        self.assert200(response)
+        self.assertIn('You need to choose a license.', str(response.data))
+
+        # test publishing draft with invalid license causes error
+        data["license_choice"] = "GPL"
+        response = self.client.post(
+            "/review/%s/edit" % review["id"],
+            data=data,
+            query_string=data,
+            follow_redirects=True
+        )
+        self.assert200(response)
+        self.assertIn('Not a valid license choice.', str(response.data))
+
+        # test publishing with a correct license choice works
+        data["license_choice"] = self.license["id"]
+        response = self.client.post(
+            "/review/%s/edit" % review["id"],
+            data=data,
+            query_string=data,
+            follow_redirects=True
+        )
+        self.assert200(response)
+        self.assertIn(self.review_text, str(response.data))
+        self.assertNotIn('You need to choose a license.', str(response.data))
+        self.assertNotIn('Not a valid license choice.', str(response.data))
+
     def test_edit_draft_without_changes(self):
         self.temporary_login(self.user)