Let reviews have multiple owners (#90)

* feat: Add better logging for auth api

* feat: Add better user api resource logging

* [build] Tweak ruff config

* refactor: Reorder data source model

this just makes things easier to read top-down

* feat: Update db models for new user/review assoc

* wip hacks

* feat: Add db migration for new review/user assoc

* fix: Rename+configure user/review relationships

* feat: Populate review-user-assoc via cli+seed

* fix: Return owner/owned user/review props working

* fix: Allow multi owners in review api

* fix: Use new review-user assoc in fulltext api

* fix: Make user/review assoc rel dynamic lazy

no idea why this works, but apparently it does

* feat: Remove owner_user_id from review

for both db model and api schema

* feat: Remove owner user id from review

in post api endpoint and test fixture data

* fix: Update review owner calls in api endpoints

* feat: Init rev-usr-assoc w/ optional role

* fix: Fix review team change logic for assoc obj

* feat: Remove old server name api arg

* lint: Auto-format random code

* feat: Add api args to set user role in team

* feat: Update review user filtering

* feat: Update user review filtering

* lint: Auto-format some code

* feat: Fix one last review user assoc filter stmt

not sure how this got missed

* build: Bump versions on gh actions

* build: Switch to ruff-only lint+fmt in pyproject

* build: Only use ruff for format+lint

* lint: Fix some linting errors

* build: Tweak ruff configuration

* build: Rename check step back to lint, oops

* build: Refine ruff linting rules

* tests: Add start of review team unit tests

* fix: Fix review team user removal

(hopefully)

* tests: Add tests for modifying review teams

.github/workflows/checks.yml

@@ -56,9 +56,9 @@ jobs:
           - 6379:6379
 
     steps:
-    - uses: actions/checkout@v3
+    - uses: actions/checkout@v4
     - name: Set up Python ${{ matrix.python-version }}
-      uses: actions/setup-python@v4
+      uses: actions/setup-python@v5
       with:
         python-version: ${{ matrix.python-version }}
         cache: "pip"
@@ -79,9 +79,9 @@ jobs:
   lint:
     runs-on: ubuntu-latest
     steps:
-    - uses: actions/checkout@v3
+    - uses: actions/checkout@v4
     - name: Set up Python
-      uses: actions/setup-python@v4
+      uses: actions/setup-python@v5
       with:
         python-version: "3.10"
         cache: "pip"
@@ -90,21 +90,15 @@ jobs:
       run: |
         python -m pip install --upgrade pip wheel
         python -m pip install -e '.[dev]'
-    - name: Check formatting with black
+    - name: Check with ruff
       run: |
-        python -m black --check --diff --verbose colandr
-    - name: Check imports with isort
-      run: |
-        python -m isort --check --diff --verbose colandr
-    - name: Check correctness with ruff
-      run: |
-        python -m ruff check --exit-zero colandr
+        python -m ruff check --output-format=github --exit-zero colandr
   types:
     runs-on: ubuntu-latest
     steps:
-    - uses: actions/checkout@v3
+    - uses: actions/checkout@v4
     - name: Set up Python
-      uses: actions/setup-python@v4
+      uses: actions/setup-python@v5
       with:
         python-version: "3.10"
         cache: "pip"

colandr/apis/auth.py

@@ -67,6 +67,7 @@ def post(self, email, password):
         user = authenticate_user(email, password)
         access_token = jwtext.create_access_token(identity=user, fresh=True)
         refresh_token = jwtext.create_refresh_token(identity=user)
+        current_app.logger.info("%s logged in", user)
         return {"access_token": access_token, "refresh_token": refresh_token}
 
 
@@ -91,7 +92,8 @@ def delete(self):
         jwt_data = jwtext.get_jwt()
         token = jwt_data["jti"]
         JWT_BLOCKLIST.add(token)
-        return ({"message": f"{current_user} successfully logged out"}, 200)
+        current_app.logger.info("%s logged out", current_user)
+        return {"message": f"{current_user} logged out"}
 
 
 @ns.route(
@@ -117,8 +119,9 @@ def get(self):
         $ curl http://localhost:5000/api/auth/refresh -X GET \
             -H "Authorization: Bearer <your_token>"
         """
-        user = jwtext.get_current_user()
-        access_token = jwtext.create_access_token(identity=user, fresh=False)
+        current_user = jwtext.get_current_user()
+        access_token = jwtext.create_access_token(identity=current_user, fresh=False)
+        current_app.logger.debug("%s refreshed JWT access token", current_user)
         return {"access_token": access_token}
 
 
@@ -174,9 +177,8 @@ def post(self, args):
             tasks.send_email.apply_async(
                 args=[[user.email], "Confirm your registration", "", html]
             )
-            current_app.logger.info(
-                "successfully sent registration email to %s", user.email
-            )
+            current_app.logger.info("registration email sent to %s", user.email)
+        current_app.logger.info("registration submitted for %s", user)
         return UserSchema().dump(user)
 
 
@@ -208,6 +210,7 @@ def get(self, token):
         user.is_confirmed = True
         db.session.commit()
         access_token = jwtext.create_access_token(identity=user)
+        current_app.logger.info("%s confirmed registration", user)
         return {"access_token": access_token}
 
 
@@ -267,6 +270,8 @@ def post(self, email):
                 tasks.send_email.apply_async(
                     args=[[user.email], "Reset your password", "", html]
                 )
+                current_app.logger.info("password reset email sent to %s", user.email)
+            current_app.logger.info("password reset submitted by %s", user)
 
 
 @ns.route(
@@ -304,7 +309,7 @@ def put(self, args, token):
             return forbidden_error(
                 "user not confirmed! please first confirm your email address."
             )
-        current_app.logger.info("password reset confirmed by %s", user.email)
+        current_app.logger.info("password reset confirmed for %s", user)
         user.password = args["password"]
         db.session.commit()
         return UserSchema().dump(user)
@@ -372,11 +377,7 @@ def authenticate_user(email: str, password: str) -> User:
     ).scalar_one_or_none()
     if user is None or user.check_password(password) is False:
         raise ValueError("invalid user email or password")
-    else:
-        current_app.logger.info(
-            "%s successfully authenticated using email='%s'", user, email
-        )
-        return user
+    return user
 
 
 def get_user_from_token(token: str) -> Optional[User]:

colandr/apis/resources/citation_imports.py

@@ -6,7 +6,7 @@
 from marshmallow import fields as ma_fields
 from marshmallow.validate import URL, Length, OneOf, Range
 from webargs.flaskparser import use_kwargs
-from werkzeug.utils import secure_filename
+# from werkzeug.utils import secure_filename
 
 from ... import tasks
 from ...extensions import db
@@ -61,7 +61,10 @@ def get(self, review_id):
             return not_found_error(f"<Review(id={review_id})> not found")
         if (
             current_user.is_admin is False
-            and current_user.reviews.filter_by(id=review_id).one_or_none() is None
+            and current_user.review_user_assoc.filter_by(
+                review_id=review_id
+            ).one_or_none()
+            is None
         ):
             return forbidden_error(
                 f"{current_user} forbidden to add citations to this review"
@@ -158,7 +161,10 @@ def post(
             return not_found_error(f"<Review(id={review_id})> not found")
         if (
             current_user.is_admin is False
-            and current_user.reviews.filter_by(id=review_id).one_or_none() is None
+            and current_user.review_user_assoc.filter_by(
+                review_id=review_id
+            ).one_or_none()
+            is None
         ):
             return forbidden_error(
                 f"{current_user} forbidden to add citations to this review"

colandr/apis/resources/citation_screenings.py

@@ -68,7 +68,9 @@ def get(self, id, fields):
             return not_found_error(f"<Citation(id={id})> not found")
         if (
             current_user.is_admin is False
-            and current_user.reviews.filter_by(id=citation.review_id).one_or_none()
+            and current_user.user_review_assoc.filter_by(
+                review_id=citation.review_id
+            ).one_or_none()
             is None
         ):
             return forbidden_error(
@@ -102,7 +104,9 @@ def delete(self, id):
             return not_found_error(f"<Citation(id={id})> not found")
         if (
             current_user.is_admin is False
-            and current_user.reviews.filter_by(id=citation.review_id).one_or_none()
+            and current_user.user_review_assoc.filter_by(
+                review_id=citation.review_id
+            ).one_or_none()
             is None
         ):
             return forbidden_error(
@@ -146,7 +150,9 @@ def post(self, args, id):
             return not_found_error(f"<Citation(id={id})> not found")
         if (
             current_user.is_admin is False
-            and current_user.reviews.filter_by(id=citation.review_id).one_or_none()
+            and current_user.user_review_assoc.filter_by(
+                review_id=citation.review_id
+            ).one_or_none()
             is None
         ):
             return forbidden_error(
@@ -300,7 +306,9 @@ def get(self, citation_id, user_id, review_id, status_counts):
                 return not_found_error(f"<Citation(id={citation_id})> not found")
             if (
                 current_user.is_admin is False
-                and citation.review.users.filter_by(id=current_user.id).one_or_none()
+                and citation.review.review_user_assoc.filter_by(
+                    user_id=current_user.id
+                ).one_or_none()
                 is None
             ):
                 return forbidden_error(
@@ -328,7 +336,10 @@ def get(self, citation_id, user_id, review_id, status_counts):
                 return not_found_error(f"<Review(id={review_id})> not found")
             if (
                 current_user.is_admin is False
-                and review.users.filter_by(id=current_user.id).one_or_none() is None
+                and review.review_user_assoc.filter_by(
+                    user_id=current_user.id
+                ).one_or_none()
+                is None
             ):
                 return forbidden_error(
                     f"{current_user} forbidden to get screenings for {review}"
@@ -412,9 +423,7 @@ def post(self, args, review_id, user_id):
                 WHERE citation_id IN ({citation_ids})
                 GROUP BY citation_id
                 ORDER BY citation_id
-                """.format(
-                citation_ids=",".join(str(cid) for cid in citation_ids)
-            )
+                """.format(citation_ids=",".join(str(cid) for cid in citation_ids))
             results = connection.execute(sa.text(query))
         studies_to_update = [
             {"id": row[0], "citation_status": assign_status(row[1], num_screeners)}

colandr/apis/resources/citations.py

@@ -63,7 +63,9 @@ def get(self, id, fields):
             return not_found_error(f"<Citation(id={id})> not found")
         if (
             current_user.is_admin is False
-            and citation.review.users.filter_by(id=current_user.id).one_or_none()
+            and citation.review.review_user_assoc.filter_by(
+                user_id=current_user.id
+            ).one_or_none()
             is None
         ):
             return forbidden_error(f"{current_user} forbidden to get this citation")
@@ -96,7 +98,9 @@ def delete(self, id):
             return not_found_error(f"<Citation(id={id})> not found")
         if (
             current_user.is_admin is False
-            and citation.review.users.filter_by(id=current_user.id).one_or_none()
+            and citation.review.review_user_assoc.filter_by(
+                user_id=current_user.id
+            ).one_or_none()
             is None
         ):
             return forbidden_error(f"{current_user} forbidden to delete this citation")
@@ -131,7 +135,9 @@ def put(self, args, id):
             return not_found_error(f"<Citation(id={id})> not found")
         if (
             current_user.is_admin is False
-            and citation.review.users.filter_by(id=current_user.id).one_or_none()
+            and citation.review.review_user_assoc.filter_by(
+                user_id=current_user.id
+            ).one_or_none()
             is None
         ):
             return forbidden_error(f"{current_user} forbidden to modify this citation")
@@ -219,7 +225,10 @@ def post(self, args, review_id, source_type, source_name, source_url, status):
             return not_found_error(f"<Review(id={review_id})> not found")
         if (
             current_user.is_admin is False
-            and current_user.reviews.filter_by(id=review_id).one_or_none() is None
+            and current_user.user_review_assoc.filter_by(
+                review_id=review_id
+            ).one_or_none()
+            is None
         ):
             return forbidden_error(
                 f"{current_user} forbidden to add citations to this review"

colandr/apis/resources/data_extractions.py

@@ -103,7 +103,9 @@ def delete(self, id, labels):
         if not extracted_data:
             return not_found_error(f"<DataExtraction(study_id={id})> not found")
         if (
-            current_user.reviews.filter_by(id=extracted_data.review_id).one_or_none()
+            current_user.user_review_assoc.filter_by(
+                review_id=extracted_data.review_id
+            ).one_or_none()
             is None
         ):
             return forbidden_error(
@@ -151,7 +153,10 @@ def put(self, args, id):
         review_id = extracted_data.review_id
         if not extracted_data:
             return not_found_error(f"<DataExtraction(study_id={id})> not found")
-        if current_user.reviews.filter_by(id=review_id).one_or_none() is None:
+        if (
+            current_user.user_review_assoc.filter_by(review_id=review_id).one_or_none()
+            is None
+        ):
             return forbidden_error(
                 "{} forbidden to modify extracted data for this study".format(
                     current_user

colandr/apis/resources/deduplicate_studies.py

@@ -54,7 +54,10 @@ def post(self, review_id):
             return not_found_error(f"<Review(id={review_id})> not found")
         if (
             current_user.is_admin is False
-            and review.users.filter_by(id=current_user.id).one_or_none() is None
+            and review.review_user_assoc.filter_by(
+                user_id=current_user.id
+            ).one_or_none()
+            is None
         ):
             return forbidden_error(
                 f"{current_user} forbidden to dedupe studies for this review"

colandr/apis/resources/exports.py

@@ -49,7 +49,10 @@ def get(self, review_id, content_type):
             return not_found_error(f"<Review(id={review_id})> not found")
         if (
             current_user.is_admin is False
-            and review.users.filter_by(id=current_user.id).one_or_none() is None
+            and review.review_user_assoc.filter_by(
+                user_id=current_user.id
+            ).one_or_none()
+            is None
         ):
             return forbidden_error(f"{current_user} forbidden to get this review")
 
@@ -199,7 +202,10 @@ def get(self, review_id, content_type):
             return not_found_error(f"<Review(id={review_id})> not found")
         if (
             current_user.is_admin is False
-            and review.users.filter_by(id=current_user.id).one_or_none() is None
+            and review.review_user_assoc.filter_by(
+                user_id=current_user.id
+            ).one_or_none()
+            is None
         ):
             return forbidden_error(f"{current_user} forbidden to get this review")