CB-410: Revisions of reviews are created when drafts are made, and they remain visible

critiquebrainz/db/review.py

@@ -273,13 +273,16 @@ def update(review_id, *, drafted, text=None, rating=None, license_id=None, langu
         updates.append("language = :language")
         updated_info["language"] = language
 
+    delete_draft_revision = False
     if is_draft is not None:
         if not drafted and is_draft:  # If trying to convert published review into draft
             raise db_exceptions.BadDataException("Converting published reviews back to drafts is not allowed.")
         if drafted and not is_draft:
             published_on = datetime.now()
             updates.append("published_on = :published_on")
             updated_info["published_on"] = published_on
+            delete_draft_revision = True
+
         updates.append("is_draft = :is_draft")
         updated_info["is_draft"] = is_draft
 
@@ -295,6 +298,10 @@ def update(review_id, *, drafted, text=None, rating=None, license_id=None, langu
             updated_info["review_id"] = review_id
             connection.execute(query, updated_info)
         db_revision.create(connection, review_id, text, rating)
+        if not is_draft:
+            db_revision.update_rating(review_id)
+        if delete_draft_revision:
+            db_revision.delete_draft_revisons(review_id)
 
     db_revision.update_rating(review_id)
 
@@ -386,7 +393,7 @@ def create(*, entity_id, entity_type, user_id, is_draft, text=None, rating=None,
         })
         review_id = result.fetchone()[0]
         db_revision.create(connection, review_id, text, rating)
-    if rating:
+    if rating and not is_draft:
         db_revision.update_rating(review_id)
 
     invalidate_ws_entity_cache(entity_id)

critiquebrainz/db/revision.py

@@ -230,3 +230,22 @@ def votes(revision_id):
             else:  # False = negative
                 revision_votes["negative"] += 1
     return revision_votes
+
+
+def delete_draft_revisons(review_id):
+    """Delete all draft revisions of a review after the review has been published.
+
+    Args:
+        review_id: ID of the review.
+    """
+    with db.engine.connect() as connection:
+        connection.execute(sqlalchemy.text("""
+            DELETE
+              FROM revision
+              USING review
+              WHERE review.id = revision.review_id
+              AND revision.review_id = :review_id
+              AND revision.timestamp < review.published_on               
+        """), {
+            "review_id": review_id,
+        })

critiquebrainz/db/test/review_test.py

@@ -138,9 +138,10 @@ def test_update(self):
         self.assertEqual(retrieved_review["text"], "Testing update")
         self.assertEqual(retrieved_review["rating"], None)
 
-        # Updating should create a new revision.
+        # Updating should create a new revision but since the review is getting published from draft,
+        # the older revision should be deleted.
         revisions = db_revision.get(retrieved_review["id"], limit=None)
-        self.assertEqual(len(revisions), 3)
+        self.assertEqual(len(revisions), 1)
         self.assertEqual(revisions[0]["timestamp"], retrieved_review["last_revision"]["timestamp"])
         self.assertEqual(revisions[0]["text"], retrieved_review["text"])
         self.assertEqual(revisions[0]["rating"], retrieved_review["rating"])

critiquebrainz/frontend/views/review.py

@@ -417,6 +417,7 @@ def edit(id):
             raise BadRequest(lazy_gettext("Changing license of a published review\
                 or converting a published review back to drafts is not allowed."))
 
+
         flash.success(gettext("Review has been updated."))
         return redirect(url_for('.entity', id=review["id"]))
     else:

critiquebrainz/frontend/views/test/test_review.py

@@ -6,6 +6,7 @@
 
 import critiquebrainz.db.license as db_license
 import critiquebrainz.db.review as db_review
+import critiquebrainz.db.revision as db_revision
 import critiquebrainz.db.users as db_users
 from critiquebrainz.db.user import User
 from critiquebrainz.frontend.testing import FrontendTestCase
@@ -374,3 +375,62 @@ def test_hide_redirect(self):
             review["entity_type"], review["entity_id"]))
         redirect_url = urlparse(response.location)
         self.assertEqual(redirect_url.path, url_for('review.entity', id=review['id']))
+
+
+    def test_publish_draft_review(self):
+        review = self.create_dummy_review(is_draft=True)
+        self.temporary_login(self.user)
+
+        revisions = db_revision.get(review["id"], limit=100)
+        self.assertEqual(len(revisions), 1)
+        self.assertEqual(revisions[0]["text"], "Testing! This text should be on the page.")
+        self.assertEqual(revisions[0]["rating"], 3)
+
+        # Now we update the draft review and check that the revision is created
+        updated_text = 'This is an updated draft review.'
+        updated_rating = 5
+
+        db_review.update(
+            review_id=review["id"],
+            drafted=True,
+            text=updated_text,
+            rating=updated_rating,
+            language=review["language"],
+            is_draft=review["is_draft"],
+        )
+
+        revisions = db_revision.get(review["id"], limit=100)
+        self.assertEqual(len(revisions), 2)
+        self.assertEqual(revisions[0]["text"], updated_text)
+        self.assertEqual(revisions[0]["rating"], updated_rating)
+        self.assertEqual(revisions[1]["text"], "Testing! This text should be on the page.")
+        self.assertEqual(revisions[1]["rating"], 3)
+
+        published_text = 'This is an updated published review.'
+        published_rating = 4
+
+        data = {
+            review["entity_type"]: review["entity_id"],
+            "state": "publish",
+            "license_choice": self.license["id"],
+            "language": 'en',
+            "agreement": 'True',
+            "text": published_text,
+            "rating": published_rating
+        }
+
+        response = self.client.post(
+            "/review/%s/edit" % review["id"],
+            data=data,
+            query_string=data,
+            follow_redirects=True
+        )
+        self.assert200(response)
+        self.assertIn(published_text, str(response.data))
+
+        # test draft revisions are deleted after review is published
+        revisions = db_revision.get(review["id"], limit=100)
+        self.assertEqual(len(revisions), 1)
+        self.assertEqual(revisions[0]["text"], published_text)
+        self.assertEqual(revisions[0]["rating"], published_rating)
+

critiquebrainz/ws/review/test/views_test.py

@@ -179,6 +179,42 @@ def test_review_modify(self):
         self.assertEqual(resp['review']['text'], data['text'])
         self.assertEqual(resp['review']['rating'], review['rating'])
 
+        draft_review = db_review.create(
+            entity_id="1eff4a06-056e-4dc7-91c4-0cbc5878f3c3",
+            entity_type='release_group',
+            user_id=self.user.id,
+            text="Testing! This is a draft review.",
+            rating=5,
+            is_draft=True,
+            license_id=self.license["id"],
+        )
+
+        # Publishing a draft review and changing the text
+        data = dict(text="This review is now published.", is_draft=False)
+        resp = self.client.post('/review/%s' % draft_review["id"], headers=self.header(self.user), data=json.dumps(data))
+        self.assert200(resp)
+
+        resp = self.client.get('/review/%s' % draft_review["id"]).json
+        self.assertEqual(resp['review']['text'], data['text'])
+        self.assertEqual(resp['review']['is_draft'], data['is_draft'])
+
+        draft_review_2 = db_review.create(
+            entity_id="c033d28b-fe43-3744-a5f8-50b30a100dcb",
+            entity_type='release_group',
+            user_id=self.user.id,
+            rating=5,
+            is_draft=True,
+            license_id=self.license["id"],
+        )
+        # Publishing a draft review without changing the text and ratings
+        data = dict(is_draft=False)
+        resp = self.client.post('/review/%s' % draft_review_2["id"], headers=self.header(self.user), data=json.dumps(data))
+        self.assert200(resp)
+
+        resp = self.client.get('/review/%s' % draft_review_2["id"]).json
+        self.assertEqual(resp['review']['text'], draft_review_2['text'])
+        self.assertEqual(resp['review']['is_draft'], data['is_draft'])
+
     def test_review_list(self):
         review = self.create_dummy_review()
         resp = self.client.get('/review/').json

critiquebrainz/ws/review/views.py

@@ -242,6 +242,7 @@ def review_modify_handler(review_id, user):
 
     :json string text: Text part of review, min length is 25, max is 5000 **(optional)**
     :json integer rating: Rating part of review, min is 1, max is 5 **(optional)**
+    :json boolean is_draft: Whether the review is a draft or not **(optional)**
 
     **NOTE:** Please provide only those parameters which need to be updated
 
@@ -262,24 +263,36 @@ def fetch_params(review):
             rating = Parser.int('json', 'rating', min=REVIEW_RATING_MIN, max=REVIEW_RATING_MAX)
         except MissingDataError:
             rating = review['rating']
-        if text is None and rating is None:
+
+        try:
+            is_draft = Parser.bool('json', 'is_draft')
+        except MissingDataError:
+            is_draft = review['is_draft']
+
+        if not review['is_draft'] and is_draft:  # If trying to convert published review into draft
+            raise InvalidRequest(desc='Converting published reviews back to drafts is not allowed.')
+
+        if text is None and rating is None and review['is_draft'] == is_draft:
             raise InvalidRequest(desc='Review must have either text or rating')
-        return text, rating
+
+        return text, rating, is_draft
 
     review = get_review_or_404(review_id)
     if review["is_hidden"]:
         raise NotFound("Review has been hidden.")
     if str(review["user_id"]) != user.id:
         raise AccessDenied
-    text, rating = fetch_params(review)
-    if (text == review['text']) and (rating == review['rating']):
+    text, rating, is_draft = fetch_params(review)
+
+    if (text == review['text']) and (rating == review['rating']) and (is_draft == review['is_draft']):
         return jsonify(message='Request processed successfully', review=dict(id=review["id"]))
 
     db_review.update(
         review_id=review_id,
         drafted=review["is_draft"],
         text=text,
-        rating=rating
+        rating=rating,
+        is_draft=is_draft
     )
     return jsonify(message='Request processed successfully',
                    review=dict(id=review["id"]))