CB-263: Update publication date of draft reviews on CritiqueBrainz when they get published #171

IsaacZanoria · 2018-01-08T00:10:44Z

add new column (publish_time) to review table which is only updated when a review is published without saving as a draft or changed from a draft to a published review. add to get_by_id() so that it gets put in the returned dictionary, and change html tempaltes to use the value from the new key

brainzbot · 2018-01-08T00:10:46Z

Can one of the admins verify this patch?

brainzbot · 2018-01-08T00:10:46Z

Can one of the admins verify this patch?

anshuman73 · 2018-01-08T17:28:46Z

critiquebrainz/db/review.py

@@ -322,7 +334,7 @@ def list_reviews(*, inc_drafts=False, inc_hidden=False, entity_id=None,
                 offset=None):
    """Get a list of reviews.

-    This function provides several filters that can be used to select a subset of reviews.
+    This function provides several  filters that can be used to select a subset of reviews.


Hi, it seems a space was added here by mistake

oh, yeah, you're right, I'll remove that right away

sorry about making more problems with one of the commits; I'm on a school computer and not having fun using the in-browser editor

ferbncode · 2018-01-08T19:38:11Z

admin/sql/create_tables.sql

+    is_hidden       BOOLEAN      NOT NULL,
+    license_id      VARCHAR      NOT NULL,
+    language        VARCHAR(3)   NOT NULL,
+    publish_time    TIMESTAMP,


Let us make that publish_date to be more clear. :)

ferbncode · 2018-01-08T19:46:15Z

critiquebrainz/db/review.py

@@ -83,6 +84,7 @@ def get_by_id(review_id):
                   review.language,
                   review.source,
                   review.source_url,
+                   review.publish_time,


Since we are adding another column to the table (for publish date), it is important to remove the subquery for creating created_time and the corresponding entry of created_time.created in SELECT, as that part is now unnecessary.

Also, for adding the Last Revised: information, you can make use of the revision.timestamp, so I would suggest it to rename the column as last_revision_timestamp.

ferbncode · 2018-01-08T19:57:54Z

critiquebrainz/db/review.py

@@ -220,6 +222,10 @@ def update(review_id, *, drafted, text=None, rating=None, license_id=None, langu
    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):


I think there is no need of the parentheses.

if drafted and not is draft: ...

ferbncode · 2018-01-09T13:10:56Z

@IsaacZanoria Since it is a schema-change PR, you need to add a file in admin/schema_changes (9.sql) for altering table review for adding a column published_on.

ferbncode · 2018-01-09T13:29:30Z

@IsaacZanoria There are more changes to be made for this PR to be merged:

Update the SCHEMA_VERSION variable in the db module.
Update _TABLES in data/dump_manager.py to include column published_on. Also, later the CB dumps need to be updated for this schema change.

ferbncode · 2018-01-09T13:43:25Z

@brainzbot test this please

ferbncode · 2018-01-09T13:52:10Z

@IsaacZanoria See the Jenkins errors to fix the tests (Pytest). You need to update the tests involving the creation of test reviews.

ferbncode

You also need to update the function db.review.list_reviews and handle cases where published_on can be Null.

ferbncode · 2018-01-09T14:41:29Z

admin/schema_changes/9.sql

@@ -0,0 +1,5 @@
+BEGIN;
+
+ALTER TABLE review ADD COLUMN published_on TIMESTAMP;


There is no need of the BEGIN and COMMIT statements here.

ferbncode · 2018-01-09T14:41:51Z

critiquebrainz/frontend/views/__init__.py

@@ -1,6 +1,7 @@
 import critiquebrainz.db.avg_rating as db_avg_rating
 import critiquebrainz.db.exceptions as db_exceptions

+


This is unnecessary. :)

sorry, ive seen a pep8 warning about "2 blank lines expected" in tests, and figured i'd just fix it

ferbncode · 2018-01-10T17:37:22Z

critiquebrainz/db/review_test.py

@@ -196,7 +196,7 @@ def test_list_reviews(self):
        self.assertEqual(count, 1)
        self.assertEqual(len(reviews), 1)

-        reviews, count = db_review.list_reviews(sort="created")
+        reviews, count = db_review.list_reviews(sort="")


I think you should put published_on as an argument here. This would test if the reviews are fetched successfully if ordered by publish date.

sorry, i already have the "published_on" argument set up, i think i was in the middle of adding it here and got distracted, and ended up leaving it blank

No problem :)

ferbncode · 2018-01-10T17:39:09Z

critiquebrainz/db/review.py

@@ -93,23 +94,12 @@ def get_by_id(review_id):
                   "user".show_gravatar,
                   "user".musicbrainz_id,
                   "user".is_blocked,
-                   created_time.created,
                   license.full_name,
                   license.info_url
              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


Please make sure that the query is right. I see that the WHERE clause is now missing. You can fix the query by adding a WHERE clause for the review_id. :)

ferbncode · 2018-01-11T15:47:15Z

I think there is a bug in pylint version, which is the reason of all the new pylint errors (pylint-dev/pylint#1609). Could you please try updating the pylint version to 1.8.1. It should fix some pylint errors. The rest of the PR looks good :)

ferbncode · 2018-01-11T16:06:37Z

Don't worry about the pylint fixes. It seems a version error and should be solved in a different PR.

Also, just saw your query on IRC regarding sorting on published_on in the db_review.list_reviews function. There is an argument include_drafts in the function set to False that should take care of our unpublished reviews. :)

paramsingh · 2018-02-19T08:10:14Z

The update script that accompanies this should also fill out the correct values in the new column added for all existing rows.

paramsingh · 2018-02-28T09:53:50Z

admin/schema_changes/9.sql

@@ -0,0 +1 @@
+ALTER TABLE review ADD COLUMN published_on TIMESTAMP;


We'd also want to fill up the published_on column with values for old reviews.

And this should be inside a transaction.

paramsingh

https://github.com/metabrainz/critiquebrainz/pull/171/files#r171191290

add new column (publish_time) to review table which is only updated when a review is published without saving as a draft or changed from a draft to a published review. add to get_by_id() so that it gets put in the returned dictionary, and change html tempaltes to use the value from the new key

…ted_time.created from db.review

…A_VERSION value also remove that "expect 2 blank lines" error PEP8 keeps on getting in views.test

all the other files had it, so i thought i'd be safe

test now sorts by new "published_on" column

Also, show both created date and published on date on user review page.

paramsingh · 2018-04-27T18:12:04Z

@brainzbot add to whitelist

paramsingh · 2018-04-30T09:01:18Z

@ferbncode, this is ready for review, would you mind taking a look at the last two commits I made?

paramsingh

OK to go for me.

🥇

ferbncode

LGTM 👍

anshuman73 reviewed Jan 8, 2018

View reviewed changes

ferbncode requested changes Jan 8, 2018

View reviewed changes

ferbncode requested changes Jan 9, 2018

View reviewed changes

ferbncode requested changes Jan 10, 2018

View reviewed changes

ferbncode mentioned this pull request Jan 12, 2018

CB-271: Automatically detect the language of reviews #166

Merged

paramsingh added the schema change label Feb 13, 2018

paramsingh reviewed Feb 28, 2018

View reviewed changes

paramsingh requested changes Mar 5, 2018

View reviewed changes

IsaacZanoria and others added 13 commits April 25, 2018 16:43

remove random space from function comments

1363be7

remove more spaces

3ec9a29

change "publish_time" to "published_on", and remove created_time/crea…

ddbd4c4

…ted_time.created from db.review

remove excess parentheses

cc71f3c

add new schema_changes file, update dump manager, and change db SCHEM…

eff3635

…A_VERSION value also remove that "expect 2 blank lines" error PEP8 keeps on getting in views.test

add extra space after commit in 9.sql

23e75e8

all the other files had it, so i thought i'd be safe

remove BEGIN and COMMIT from 9.sql and update list_reviews function

a5eebd5

update review_test.py

c1297a3

test now sorts by new "published_on" column

replace all review.created with review.published_on

c28af48

replace more 'created's with 'published_on'

0925715

fix review_test sort, add back WHERE clause to get_by_id

1990e1b

Update schema change script

52aa7eb

Also, show both created date and published on date on user review page.

paramsingh force-pushed the CB-263 branch from dfc5287 to 52aa7eb Compare April 27, 2018 18:04

Missed a semicolon

1d87094

paramsingh approved these changes Apr 30, 2018

View reviewed changes

ferbncode approved these changes Apr 30, 2018

View reviewed changes

paramsingh merged commit 617bef4 into metabrainz:master Jun 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CB-263: Update publication date of draft reviews on CritiqueBrainz when they get published #171

CB-263: Update publication date of draft reviews on CritiqueBrainz when they get published #171

IsaacZanoria commented Jan 8, 2018

brainzbot commented Jan 8, 2018

brainzbot commented Jan 8, 2018

anshuman73 Jan 8, 2018

IsaacZanoria Jan 8, 2018

IsaacZanoria Jan 8, 2018

ferbncode Jan 8, 2018

ferbncode Jan 8, 2018

ferbncode Jan 8, 2018

ferbncode Jan 8, 2018

ferbncode commented Jan 9, 2018 •

edited

ferbncode commented Jan 9, 2018

ferbncode commented Jan 9, 2018

ferbncode commented Jan 9, 2018 •

edited

ferbncode left a comment

ferbncode Jan 9, 2018

ferbncode Jan 9, 2018

IsaacZanoria Jan 9, 2018

ferbncode Jan 10, 2018

IsaacZanoria Jan 11, 2018

ferbncode Jan 11, 2018

ferbncode Jan 10, 2018

ferbncode commented Jan 11, 2018

ferbncode commented Jan 11, 2018

paramsingh commented Feb 19, 2018

paramsingh Feb 28, 2018

paramsingh left a comment

paramsingh commented Apr 27, 2018

paramsingh commented Apr 30, 2018

paramsingh left a comment

ferbncode left a comment

		@@ -0,0 +1,5 @@
		BEGIN;

		ALTER TABLE review ADD COLUMN published_on TIMESTAMP;

		@@ -1,6 +1,7 @@
		import critiquebrainz.db.avg_rating as db_avg_rating
		import critiquebrainz.db.exceptions as db_exceptions

		@@ -0,0 +1 @@
		ALTER TABLE review ADD COLUMN published_on TIMESTAMP;

CB-263: Update publication date of draft reviews on CritiqueBrainz when they get published #171

CB-263: Update publication date of draft reviews on CritiqueBrainz when they get published #171

Conversation

IsaacZanoria commented Jan 8, 2018

brainzbot commented Jan 8, 2018

brainzbot commented Jan 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferbncode commented Jan 9, 2018 • edited

ferbncode commented Jan 9, 2018

ferbncode commented Jan 9, 2018

ferbncode commented Jan 9, 2018 • edited

ferbncode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferbncode commented Jan 11, 2018

ferbncode commented Jan 11, 2018

paramsingh commented Feb 19, 2018

Choose a reason for hiding this comment

paramsingh left a comment

Choose a reason for hiding this comment

paramsingh commented Apr 27, 2018

paramsingh commented Apr 30, 2018

paramsingh left a comment

Choose a reason for hiding this comment

ferbncode left a comment

Choose a reason for hiding this comment

ferbncode commented Jan 9, 2018 •

edited

ferbncode commented Jan 9, 2018 •

edited