CB-230: Migrate review related code off the ORM #91

ferbncode · 2017-03-19T20:12:52Z

Functions and tests for migrating reviews off the ORM. Changes have been made to the frontend component to use the migrated code. I have added the following functions in db/review.py and the corresponding tests (db/review_test.py):

get_by_id for getting review by id.
hide and unhide for hiding and unhiding reviews.
get_count for review count.
update for updating a reviews info.
create for creating a review.
delete for deleting a review.
list for list reviews and filter them on various filters.
get_popular for getting popular reviews.
revision.create for creating a revision.
votes for getting votes of a revision.

gentlecat

This is quite large, so I didn't have enough time to look at all of the changes yet.

gentlecat · 2017-03-21T13:33:54Z

critiquebrainz/db/review.py

+    review["id"] = str(review["id"])
+    review["entity_id"] = str(review["entity_id"])
+    review["last_updated"] = review["last_revision"]["timestamp"]
+    review['last_revision']['review_id'] = str(review['last_revision']['review_id'])


A minor thing, but you should stick to one type of quotation marks within a file.

gentlecat · 2017-03-21T13:37:43Z

critiquebrainz/db/review.py

+        {
+            "id": uuid,
+            "entity_id": uuid,
+            "entity_type": 'release_group', 'event', 'place',


You should be consistent with the way output values are documented. All the other ones have "name": type structure, but this one is different. How about something like "entity_type": str {"release_group", "event", "place"}?

gentlecat · 2017-03-21T13:45:01Z

critiquebrainz/db/review.py

+            "last_revision: dict,
+            "votes": dict,
+            "rating": int,
+            "text": str, "created": datetime,


Looks like this needs to be split up into multiple lines.

gentlecat · 2017-03-21T13:46:18Z

critiquebrainz/db/review.py

+                   is_blocked,
+                   created_time.created,
+                   license.full_name,
+                   license.info_url


I think it would be better to reference each table explicitly to make it easier to understand. For example, "user".display_name instead of just display_name.

gentlecat · 2017-03-21T13:56:34Z

critiquebrainz/db/review.py

+
+        review = result.fetchone()
+        if not review:
+            raise db_exceptions.NoDataFoundException("No review found with review ID: {}".format(review_id))


How about Can't find review with ID: {id}?

gentlecat · 2017-03-21T14:14:54Z

critiquebrainz/db/review.py

+                 WHEN vote='t' THEN 1
+                 ELSE 0
+                END
+               ) AS votes_positive_count,


A couple of problems here. The goal of splitting parts of a query into multiple rows is to make it more readable. It doesn't mean that everything needs to be like that. This query doesn't even fit on my screen.

Statement like this is still readable:

SUM( CASE WHEN vote='t' THEN 1 ELSE 0 END ) AS votes_positive_count,

And make sure that you use 4 spaces for indentation, not 1.

gentlecat · 2017-03-21T14:16:18Z

critiquebrainz/db/review.py

+          JOIN "user"
+            ON review.user_id = "user".id
+          JOIN license
+            ON license.id = license_id


Same with these JOINs. This is fine:

JOIN license ON license.id = license_id

gentlecat · 2017-03-21T14:18:39Z

critiquebrainz/db/review.py

+                 AND latest.latest_timestamp = revision.timestamp
+               ) AS latest_revision
+            ON review.id = latest_revision.review_id
+            {}


Placeholder names should be explicitly specified: {where_clause} instead of {}, etc.

gentlecat · 2017-03-21T14:27:04Z

critiquebrainz/db/vote.py

@@ -56,6 +56,7 @@ def submit(user_id, revision_id, vote):
            "vote": vote,
            "rated_at": datetime.utcnow(),
        })
+        return get(user_id, revision_id)


Is data that returned from submit used somewhere?

Also don't forget to document the return value.

gentlecat · 2017-03-21T14:28:08Z

critiquebrainz/frontend/views/index.py


    # Statistics
-    review_count = format_number(Review.get_count(is_draft = False))
+    review_count = format_number(db_review.get_count(is_draft = False))


There should've been no spaces around =.

gentlecat

OK, finally looked over most things. Looks great.
I pointed out several other problems that need to be fixed.

gentlecat · 2017-03-24T00:35:03Z

critiquebrainz/db/review.py

+             WHERE id = :review_id
+        """), {
+            "review_id": review_id,
+        })


Can hide and unhide be combined into one function? Something like set_hidden_state(review_id, is_hidden).

gentlecat · 2017-03-24T00:35:43Z

critiquebrainz/db/review.py

+
+
+def hide(review_id):
+    """Hide review given the review id.


Hide review with a given ID. would be a better description.

gentlecat · 2017-03-24T00:38:53Z

critiquebrainz/db/review.py

+import critiquebrainz.db.revision as db_revision
+from critiquebrainz.db.user import User
+import critiquebrainz.db.users as db_users
+from critiquebrainz.db.user import User


I see some duplicate imports there.

gentlecat · 2017-03-24T00:40:38Z

critiquebrainz/db/review.py

+
+
+def to_dict(review, confidential=False):
+    review["user"] = User(db_users.get_by_id(review.pop("user_id")))


Is there any reason to use a user class instead of providing a dictionary with user data? If it's not necessary, maybe it's better to be consistent with the way the rest of data is returned?

This is also the case in other places. Can you help me understand how it's used?

In review_credit macro present in macros.html, review.user.avatar is used to show user avatar which is a property in the User class. The macro is used in popular reviews section and browse reviews pages.

Got it. Can improve that part later, if necessary.

gentlecat · 2017-03-24T00:48:01Z

critiquebrainz/db/review.py

+
+def update(*, review_id, drafted, text, license_id=None,
+           language=None, is_draft=None):
+    """Update contents of this review.


Update contents of a review.

gentlecat · 2017-03-24T01:03:27Z

critiquebrainz/db/review.py

+           "edits": 0,
+           "is_draft": is_draft,
+           "is_hidden": False,
+           "language": language,


It might be a good idea to check that provided language is one of the supported ones.

gentlecat · 2017-03-24T01:05:34Z

critiquebrainz/db/review.py

+
+def list_reviews(*, inc_drafts=False, inc_hidden=False, entity_id=None,
+                 entity_type=None, license_id=None, user_id=None,
+                 language=None, exclude=None, sort=None, limit=None,


It would be a good idea to provide a reasonable default limit to avoid issues in case it's not provided. 20 sounds like a good number.

gentlecat · 2017-03-24T01:07:14Z

critiquebrainz/db/review.py

+        license_id (str): License of the returned reviews.
+        inc_drafts (bool): True if reviews marked as drafts should be included. False if not.
+        inc_hidden (bool): True if reviews marked as hidden should be included. False if not.
+        exclude (list): List of id of reviews to exclude.


List of reviews (their IDs) to exclude from results.

gentlecat · 2017-03-24T01:09:05Z

critiquebrainz/db/review.py

+
+    Returns:
+        Dictionary of reviews.
+        Total number of reviews.


Total number of reviews that match specified filters.

gentlecat · 2017-03-24T01:21:05Z

critiquebrainz/frontend/views/review.py

+            drafted=review["is_draft"],
+            text=form.text.data,
+            is_draft=(form.state.data == 'draft'),
+            license_id=license_choice, language=form.language.data,


Can you put language argument on a new line?

gentlecat · 2017-04-13T13:29:23Z

critiquebrainz/db/review.py

-        if not entity_type or not entity_id:
-          raise TypeError("Entity Type or ID not defined")
+    if language not in supported_languages:
+        raise TypeError("Language: {} is not supported".format(language))


This doesn't look like a TypeError. Take a look its description: https://docs.python.org/3.6/library/exceptions.html#TypeError. Someone might pass an invalid language as a string.

ValueError would be the right one to use here.

gentlecat · 2017-04-13T13:32:32Z

critiquebrainz/frontend/views/review.py

@@ -365,7 +366,7 @@ def hide(id):

    form = AdminActionForm()
    if form.validate_on_submit():
-        db_review.hide(review["id"])
+        db_review.set_hidden_state(review["id"], is_hidden="True")


The is_hidden argument should be a bool. Pretty sure you can pass it straight to psycopg too.

gentlecat · 2017-04-13T13:32:42Z

critiquebrainz/frontend/views/review.py

@@ -382,6 +383,6 @@ def hide(id):
 @admin_view
 def unhide(id):
    review = db_review.get_or_404(id)
-    db_review.unhide(review["id"])
+    db_review.set_hidden_state(review["id"], is_hidden="False")


Same issue with is_hidden here.

If you have to accept boolean arguments as strings in an interface, it's a good sign that something is not right. :)

gentlecat · 2017-04-13T13:36:10Z

critiquebrainz/db/review.py

+for lang in list(pycountry.languages):
+    if 'iso639_1_code' in dir(lang):
+        supported_languages.append(lang.iso639_1_code)
+


Minor thing: there should be two spaces between functions and other code.

gentlecat · 2017-04-13T13:47:29Z

critiquebrainz/db/review.py

+
+    if license_id is not None:
+        if not drafted: # If trying to convert published review into draft.
+            raise BadRequest(lazy_gettext("Changing license of a published review is not allowed."))


One other important thing that I missed. We should try to avoid making Flask views and database access code closely tied. That means not using exceptions and functions from Flask.

Modules in the critiquebrainz.db package should raise exceptions from critiquebrianz.db.exceptions. I understand that this is not the old implementation, but that's the perfect time to make this improvement. This would mean we could easily use DB access code without Flask.

gentlecat · 2017-04-13T13:50:26Z

critiquebrainz/db/review.py

+    if 'iso639_1_code' in dir(lang):
+        supported_languages.append(lang.iso639_1_code)
+
+def get_or_404(review_id):


We probably shouldn't have this function here. As I described in another comment, this makes db package closely tied to Flask itself. This function can be moved into packages with views if it's used more than once.

Functions and tests for directly accessing review information.

Corrections in list reviews, popular reviews, submit votes and used keyword only arguments for functions, also made syntax corrections.

Refactor create_review function, corrections in update function, combine hide and unhide functions.

gentlecat · 2017-04-17T17:00:48Z

Looks like there's a small conflict with another change. Would you mind making an update please?

gentlecat · 2017-04-17T19:17:31Z

Great. 👍

gentlecat · 2017-06-30T11:23:12Z

critiquebrainz/db/review.py

+        result = connection.execute(sqlalchemy.text("""
+            INSERT INTO review
+            VALUES (:id, :entity_id, :entity_type, :user_id, :edits, :is_draft,
+            :is_hidden, :license_id, :language, :source, :source_url)


This way of insertion is unsafe and caused CB-258. Order of columns needs to be explicitly specified. Here you are making an assumption that entity_type is third, which it might be when a new instance of CB database is created, but it isn't in the production database.

gentlecat · 2017-06-30T11:25:26Z

critiquebrainz/db/review.py

+    """
+    cache_key = cache.gen_key("popular_reviews", limit)
+    reviews = cache.get(cache_key, REVIEW_CACHE_NAMESPACE)
+    reviews = None


What's the point of retrieving reviews from cache and then getting rid of them in the next statement?

gentlecat · 2017-06-30T11:26:24Z

Next time make sure that your PR is small and easy to review. Otherwise, as evidenced here, this causes many issues.

ferbncode force-pushed the migrate-reviews branch from 9f12391 to abde3cb Compare March 19, 2017 20:34

gentlecat suggested changes Mar 21, 2017

View reviewed changes

ferbncode force-pushed the migrate-reviews branch 2 times, most recently from 887259c to 9032a75 Compare March 23, 2017 16:52

gentlecat suggested changes Mar 24, 2017

View reviewed changes

ferbncode force-pushed the migrate-reviews branch from 9032a75 to 04d56a8 Compare March 25, 2017 04:14

gentlecat suggested changes Apr 13, 2017

View reviewed changes

ferbncode force-pushed the migrate-reviews branch from 9bbfcea to 7cc6b7c Compare April 13, 2017 19:19

ferbncode added 5 commits April 14, 2017 00:51

CB-230: Migrate reviews off the ORM.

f63460f

Functions and tests for directly accessing review information.

Changes in tests in db to use migrated code.

7f1bc69

Changes in views to use migrated code.

57dc2f1

Syntax correction, use keyword only arguments.

83ac763

Corrections in list reviews, popular reviews, submit votes and used keyword only arguments for functions, also made syntax corrections.

Changes to check language support, create function.

dbcfada

Refactor create_review function, corrections in update function, combine hide and unhide functions.

ferbncode force-pushed the migrate-reviews branch from 7cc6b7c to 848b1a5 Compare April 13, 2017 19:22

Removed flask related code from db, Syntax fixes.

848b1a5

gentlecat approved these changes Apr 17, 2017

View reviewed changes

ferbncode added 2 commits April 17, 2017 23:31

Merge branch 'master' into migrate-reviews

d6845ba

Resolve Merge Conflicts.

285278a

ferbncode force-pushed the migrate-reviews branch from 3410d1a to 285278a Compare April 17, 2017 18:54

gentlecat merged commit e11662b into metabrainz:master Apr 17, 2017

gentlecat reviewed Jun 30, 2017

View reviewed changes



		def to_dict(review, confidential=False):
		review["user"] = User(db_users.get_by_id(review.pop("user_id")))

CB-230: Migrate review related code off the ORM #91

CB-230: Migrate review related code off the ORM #91

Conversation

ferbncode commented Mar 19, 2017 • edited

gentlecat 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

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

gentlecat 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

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

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

gentlecat commented Apr 17, 2017

gentlecat commented Apr 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gentlecat commented Jun 30, 2017

ferbncode commented Mar 19, 2017 •

edited