diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index 0dffbc88..2aae1422 100644 --- a/.github/workflows/checks.yml +++ b/.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" diff --git a/colandr/apis/auth.py b/colandr/apis/auth.py index c2a6bdb2..cd6709bb 100644 --- a/colandr/apis/auth.py +++ b/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 " """ - 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]: diff --git a/colandr/apis/resources/citation_imports.py b/colandr/apis/resources/citation_imports.py index e8bd0bca..1599ad14 100644 --- a/colandr/apis/resources/citation_imports.py +++ b/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" 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" 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" diff --git a/colandr/apis/resources/citation_screenings.py b/colandr/apis/resources/citation_screenings.py index 2b81b44b..ddc5077a 100644 --- a/colandr/apis/resources/citation_screenings.py +++ b/colandr/apis/resources/citation_screenings.py @@ -68,7 +68,9 @@ def get(self, id, fields): return not_found_error(f" 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" 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" 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" 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" 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)} diff --git a/colandr/apis/resources/citations.py b/colandr/apis/resources/citations.py index 2ced6707..25572399 100644 --- a/colandr/apis/resources/citations.py +++ b/colandr/apis/resources/citations.py @@ -63,7 +63,9 @@ def get(self, id, fields): return not_found_error(f" 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" 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" 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" 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" diff --git a/colandr/apis/resources/data_extractions.py b/colandr/apis/resources/data_extractions.py index 4e02f27c..d1ce53ab 100644 --- a/colandr/apis/resources/data_extractions.py +++ b/colandr/apis/resources/data_extractions.py @@ -103,7 +103,9 @@ def delete(self, id, labels): if not extracted_data: return not_found_error(f" 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" 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 diff --git a/colandr/apis/resources/deduplicate_studies.py b/colandr/apis/resources/deduplicate_studies.py index a1796ea4..6b9df9b3 100644 --- a/colandr/apis/resources/deduplicate_studies.py +++ b/colandr/apis/resources/deduplicate_studies.py @@ -54,7 +54,10 @@ def post(self, review_id): return not_found_error(f" 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" diff --git a/colandr/apis/resources/exports.py b/colandr/apis/resources/exports.py index 1664f0a3..e7bc2ec6 100644 --- a/colandr/apis/resources/exports.py +++ b/colandr/apis/resources/exports.py @@ -49,7 +49,10 @@ def get(self, review_id, content_type): return not_found_error(f" 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" 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") diff --git a/colandr/apis/resources/fulltext_screenings.py b/colandr/apis/resources/fulltext_screenings.py index 55781826..81abec96 100644 --- a/colandr/apis/resources/fulltext_screenings.py +++ b/colandr/apis/resources/fulltext_screenings.py @@ -66,7 +66,9 @@ def get(self, id, fields): return not_found_error(" not found".format(id)) if ( current_user.is_admin is False - and current_user.reviews.filter_by(id=fulltext.review_id).one_or_none() + and current_user.review_user_assoc.filter_by( + review_id=fulltext.review_id + ).one_or_none() is None ): return forbidden_error( @@ -99,7 +101,12 @@ def delete(self, id): fulltext = db.session.get(Fulltext, id) if not fulltext: return not_found_error(" not found".format(id)) - if current_user.reviews.filter_by(id=fulltext.review_id).one_or_none() is None: + if ( + current_user.review_user_assoc.filter_by( + review_id=fulltext.review_id + ).one_or_none() + is None + ): return forbidden_error( "{} forbidden to delete fulltext screening for this review".format( current_user @@ -145,7 +152,9 @@ def post(self, args, id): return not_found_error(" not found".format(id)) if ( current_user.is_admin is False - and current_user.reviews.filter_by(id=fulltext.review_id).one_or_none() + and current_user.review_user_assoc.filter_by( + review_id=fulltext.review_id + ).one_or_none() is None ): return forbidden_error( @@ -306,7 +315,9 @@ def get(self, fulltext_id, user_id, review_id, status_counts): ) if ( current_user.is_admin is False - and fulltext.review.users.filter_by(id=current_user.id).one_or_none() + and fulltext.review.review_user_assoc.filter_by( + user_id=current_user.id + ).one_or_none() is None ): return forbidden_error( @@ -336,7 +347,10 @@ def get(self, fulltext_id, user_id, review_id, status_counts): return not_found_error(" not found".format(review_id)) 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( "{} forbidden to get screenings for {}".format(current_user, review) @@ -422,9 +436,7 @@ def post(self, args, review_id, user_id): WHERE fulltext_id IN ({fulltext_ids}) GROUP BY fulltext_id ORDER BY fulltext_id - """.format( - fulltext_ids=",".join(str(cid) for cid in fulltext_ids) - ) + """.format(fulltext_ids=",".join(str(cid) for cid in fulltext_ids)) results = connection.execute(sa.text(query)) studies_to_update = [ {"id": row[0], "fulltext_status": assign_status(row[1], num_screeners)} diff --git a/colandr/apis/resources/fulltext_uploads.py b/colandr/apis/resources/fulltext_uploads.py index f2a69bea..e3224436 100644 --- a/colandr/apis/resources/fulltext_uploads.py +++ b/colandr/apis/resources/fulltext_uploads.py @@ -86,7 +86,10 @@ def get(self, id, review_id): return not_found_error(f" 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's fulltexts" @@ -137,7 +140,9 @@ def post(self, id, uploaded_file): return not_found_error(f" not found") if ( current_user.is_admin is False - and current_user.reviews.filter_by(id=fulltext.review_id).one_or_none() + and current_user.user_review_assoc.filter_by( + review_id=fulltext.review_id + ).one_or_none() is None ): return forbidden_error( @@ -210,7 +215,9 @@ def delete(self, id): return not_found_error(f" not found") if ( current_user.is_admin is False - and current_user.reviews.filter_by(id=fulltext.review_id).one_or_none() + and current_user.user_review_assoc.filter_by( + review_id=fulltext.review_id + ).one_or_none() is None ): return forbidden_error( @@ -228,7 +235,7 @@ def delete(self, id): ) try: os.remove(filepath) - except OSError as e: + except OSError: msg = "error removing uploaded full-text file from disk" current_app.logger.exception(msg + "\n") return not_found_error(msg) diff --git a/colandr/apis/resources/fulltexts.py b/colandr/apis/resources/fulltexts.py index c15033e3..d09c767b 100644 --- a/colandr/apis/resources/fulltexts.py +++ b/colandr/apis/resources/fulltexts.py @@ -57,7 +57,9 @@ def get(self, id, fields): return not_found_error(f" not found") if ( current_user.is_admin is False - and fulltext.review.users.filter_by(id=current_user.id).one_or_none() + and fulltext.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 fulltext") @@ -90,7 +92,9 @@ def delete(self, id): return not_found_error(f" not found") if ( current_user.is_admin is False - and fulltext.review.users.filter_by(id=current_user.id).one_or_none() + and fulltext.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 fulltext") diff --git a/colandr/apis/resources/review_exports.py b/colandr/apis/resources/review_exports.py index 98855a6c..c30d5a48 100644 --- a/colandr/apis/resources/review_exports.py +++ b/colandr/apis/resources/review_exports.py @@ -54,7 +54,10 @@ def get(self, id): return not_found_error(f" 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") # get counts by step, i.e. prisma @@ -160,7 +163,10 @@ def get(self, id): return not_found_error(f" 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") diff --git a/colandr/apis/resources/review_plans.py b/colandr/apis/resources/review_plans.py index 47d061d0..fdbcd02b 100644 --- a/colandr/apis/resources/review_plans.py +++ b/colandr/apis/resources/review_plans.py @@ -64,7 +64,10 @@ def get(self, id, fields): return not_found_error(f" 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 plan") if fields and "id" not in fields: @@ -108,7 +111,7 @@ def delete(self, id, fields): review = db.session.get(Review, id) if not review: return not_found_error(f" not found") - if current_user.is_admin is False and review.owner is not current_user: + if current_user.is_admin is False and current_user not in review.owners: return forbidden_error( f"{current_user} forbidden to delete this review plan" ) @@ -169,7 +172,7 @@ def put(self, args, id, fields): review = db.session.get(Review, id) if not review: return not_found_error(f" not found") - if current_user.is_admin is False and review.owner is not current_user: + if current_user.is_admin is False and current_user not in review.owners: return forbidden_error( f"{current_user} forbidden to create this review plan" ) diff --git a/colandr/apis/resources/review_progress.py b/colandr/apis/resources/review_progress.py index 220be362..6ffea68a 100644 --- a/colandr/apis/resources/review_progress.py +++ b/colandr/apis/resources/review_progress.py @@ -89,7 +89,10 @@ def get(self, id, step, user_view): return not_found_error(f" 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 review progress") if step in ("planning", "all"): @@ -141,9 +144,7 @@ def get(self, id, step, user_view): ) AS t WHERE dedupe_status = 'not_duplicate' -- this is necessary! GROUP BY user_status; - """.format( - user_id=current_user.id, review_id=id - ) + """.format(user_id=current_user.id, review_id=id) progress = dict(row for row in db.engine.execute(sa.text(query))) progress = { status: progress.get(status, 0) @@ -186,9 +187,7 @@ def get(self, id, step, user_view): ) AS t WHERE citation_status = 'included' -- this is necessary! GROUP BY user_status; - """.format( - user_id=current_user.id, review_id=id - ) + """.format(user_id=current_user.id, review_id=id) progress = dict(row for row in db.engine.execute(sa.text(query))) progress = { status: progress.get(status, 0) diff --git a/colandr/apis/resources/review_teams.py b/colandr/apis/resources/review_teams.py index 7a814bd7..2e50a0b3 100644 --- a/colandr/apis/resources/review_teams.py +++ b/colandr/apis/resources/review_teams.py @@ -11,7 +11,7 @@ from ... import tasks from ...extensions import db from ...lib import constants -from ...models import Review, User +from ...models import Review, ReviewUserAssoc, User from .. import auth from ..errors import bad_request_error, forbidden_error, not_found_error from ..schemas import UserSchema @@ -63,16 +63,20 @@ def get(self, id, fields): return not_found_error(f" 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") if fields and "id" not in fields: fields.append("id") users = UserSchema(many=True, only=fields).dump(review.users) - owner_user_id = review.owner_user_id + owner_user_ids = {owner.id for owner in review.owners} + # TODO: don't always include is-owner, maybe? + # if fields is None or "is_owner" in fields: for user in users: - if user["id"] == owner_user_id: - user["is_owner"] = True + user["is_owner"] = user["id"] in owner_user_ids current_app.logger.debug("got %s team members for %s", len(users), review) return users @@ -82,8 +86,8 @@ def get(self, id, fields): "in": "query", "type": "string", "required": True, - "enum": ["add", "invite", "remove", "make_owner"], - "description": "add, invite, remove, or promote to owner a particular user", + "enum": ["add", "invite", "remove", "make_owner", "set_role"], + "description": "add, invite, remove, or set the role for a particular user", }, "user_id": { "in": "query", @@ -98,11 +102,11 @@ def get(self, id, fields): "format": "email", "description": "email address of the user to invite", }, - "server_name": { + "user_role": { "in": "query", "type": "string", - "default": None, - "description": 'name of server used to build confirmation url, e.g. "http://www.colandrapp.com"', + "enum": ["member", "owner"], + "description": "type of role to set for user on review", }, }, responses={ @@ -122,24 +126,27 @@ def get(self, id, fields): @use_kwargs( { "action": ma_fields.Str( - required=True, validate=OneOf(["add", "invite", "remove", "make_owner"]) + required=True, + validate=OneOf(["add", "invite", "remove", "make_owner", "set_role"]), ), "user_id": ma_fields.Int( load_default=None, validate=Range(min=1, max=constants.MAX_INT) ), "user_email": ma_fields.Email(load_default=None), - "server_name": ma_fields.Str(load_default=None), + "user_role": ma_fields.Str( + validate=OneOf(["member", "owner"]), load_default=None + ), }, location="query", ) @jwtext.jwt_required(fresh=True) - def put(self, id, action, user_id, user_email, server_name): - """add, invite, remove, or promote a review team member""" + def put(self, id, action, user_id, user_email, user_role): + """add, invite, remove, or set the role for a particular user""" current_user = jwtext.get_current_user() review = db.session.get(Review, id) if not review: return not_found_error(f" not found") - if current_user.is_admin is False and review.owner is not current_user: + if current_user.is_admin is False and current_user not in review.owners: return forbidden_error( f"{current_user} forbidden to modify this review team" ) @@ -186,28 +193,30 @@ def put(self, id, action, user_id, user_email, server_name): tasks.send_email.apply_async( args=[[user.email], "Let's collaborate!", "", html] ) - elif action == "make_owner": + elif action in ("make_owner", "set_role"): if user is None: return not_found_error("no user found with given id or email") - review.owner_user_id = user_id - review.owner = user + rua = review.review_user_assoc.filter_by(user_id=user_id).one_or_none() + if rua is None: + return not_found_error("no such user found with access to this review") + else: + rua.user_role = "owner" if action == "make_owner" else user_role elif action == "remove": if user is None: return not_found_error("no user found with given id or email") - if user_id == review.owner_user_id: - return forbidden_error( - "current review owner can not be removed from team" - ) - if review_users.filter_by(id=user_id).one_or_none() is not None: - review_users.remove(user) + review_owners = review.owners + if user in review_owners and len(review_owners) == 1: + return forbidden_error("only review owner can not be removed from team") + rua = review.review_user_assoc.filter_by(user_id=user_id).one_or_none() + if rua is not None: + db.session.delete(rua) db.session.commit() current_app.logger.info("for %s, %s %s", review, action, user) users = UserSchema(many=True).dump(review.users) - owner_user_id = review.owner_user_id + owner_user_ids = {owner.id for owner in review.owners} for user in users: - if user["id"] == owner_user_id: - user["is_owner"] = True + user["is_owner"] = user["id"] in owner_user_ids return users @@ -260,8 +269,7 @@ def get(self, id, token): db.session.commit() current_app.logger.info("invitation to %s confirmed by %s", review, user.email) users = UserSchema(many=True).dump(review.users) - owner_user_id = review.owner_user_id + owner_user_ids = {owner.id for owner in review.owners} for user in users: - if user["id"] == owner_user_id: - user["is_owner"] = True + user["is_owner"] = user["id"] in owner_user_ids return users diff --git a/colandr/apis/resources/reviews.py b/colandr/apis/resources/reviews.py index 3779a2df..305272de 100644 --- a/colandr/apis/resources/reviews.py +++ b/colandr/apis/resources/reviews.py @@ -13,7 +13,7 @@ from ...extensions import db from ...lib import constants -from ...models import Review +from ...models import Review, ReviewUserAssoc from ..errors import forbidden_error, not_found_error from ..schemas import ReviewSchema from ..swagger import review_model @@ -65,7 +65,10 @@ def get(self, id, fields): return not_found_error(f" not found") if ( not current_user.is_admin - 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") if fields and "id" not in fields: @@ -95,7 +98,7 @@ def delete(self, id): review = db.session.get(Review, id) if not review: return not_found_error(f" not found") - if not current_user.is_admin and review.owner is not current_user: + if not current_user.is_admin and current_user not in review.owners: return forbidden_error(f"{current_user} forbidden to delete this review") db.session.delete(review) db.session.commit() @@ -133,7 +136,7 @@ def put(self, args, id): review = db.session.get(Review, id) if not review: return not_found_error(f" not found") - if not current_user.is_admin and review.owner is not current_user: + if not current_user.is_admin and current_user not in review.owners: return forbidden_error(f"{current_user} forbidden to update this review") for key, value in args.items(): if key is missing: @@ -202,15 +205,15 @@ def get(self, fields, _review_ids): expect=(review_model, "review data to be created"), responses={200: "review was created"}, ) - @use_args(ReviewSchema(partial=["owner_user_id"]), location="json") + @use_args(ReviewSchema(), location="json") @jwtext.jwt_required() def post(self, args): """create new review""" current_user = jwtext.get_current_user() name = args.pop("name") - review = Review(name, current_user.id, **args) - current_user.owned_reviews.append(review) - current_user.reviews.append(review) + review = Review(name, **args) + # TODO: do we want to allow admins to set other users as owners? + review.review_user_assoc.append(ReviewUserAssoc(review, current_user, "owner")) db.session.add(review) db.session.commit() current_app.logger.info("inserted %s", review) diff --git a/colandr/apis/resources/studies.py b/colandr/apis/resources/studies.py index 8314ca3d..af98b2a1 100644 --- a/colandr/apis/resources/studies.py +++ b/colandr/apis/resources/studies.py @@ -66,7 +66,10 @@ def get(self, id, fields): return not_found_error(f" not found") if ( current_user.is_admin is False - and study.review.users.filter_by(id=current_user.id).one_or_none() is None + and study.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 study") if fields and "id" not in fields: @@ -98,7 +101,10 @@ def delete(self, id): return not_found_error(f" not found") if ( current_user.is_admin is False - and study.review.users.filter_by(id=current_user.id).one_or_none() is None + and study.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 study") db.session.delete(study) @@ -132,7 +138,10 @@ def put(self, args, id): return not_found_error(f" not found") if ( current_user.is_admin is False - and study.review.users.filter_by(id=current_user.id).one_or_none() is None + and study.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 study") for key, value in args.items(): @@ -288,7 +297,10 @@ def get( return not_found_error(f" 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 get studies from this review" @@ -323,9 +335,7 @@ def get( t.dedupe_status = 'not_duplicate' -- this is necessary! AND t.citation_status NOT IN ('excluded', 'included', 'conflict') AND (t.citation_status = 'not_screened' OR NOT {user_id} = ANY(t.user_ids)) - """.format( - user_id=current_user.id - ) + """.format(user_id=current_user.id) query = query.filter(Study.id.in_(text(stmt))) elif citation_status == "awaiting_coscreener": stmt = """ @@ -341,9 +351,7 @@ def get( WHERE t.citation_status = 'screened_once' AND {user_id} = ANY(t.user_ids) - """.format( - user_id=current_user.id - ) + """.format(user_id=current_user.id) query = query.filter(Study.id.in_(text(stmt))) if fulltext_status is not None: @@ -368,9 +376,7 @@ def get( t.citation_status = 'included' -- this is necessary! AND t.fulltext_status NOT IN ('excluded', 'included', 'conflict') AND (t.fulltext_status = 'not_screened' OR NOT {user_id} = ANY(t.user_ids)) - """.format( - user_id=current_user.id - ) + """.format(user_id=current_user.id) query = query.filter(Study.id.in_(text(stmt))) elif fulltext_status == "awaiting_coscreener": stmt = """ @@ -386,18 +392,14 @@ def get( WHERE t.fulltext_status = 'screened_once' AND {user_id} = ANY(t.user_ids) - """.format( - user_id=current_user.id - ) + """.format(user_id=current_user.id) query = query.filter(Study.id.in_(text(stmt))) if data_extraction_status is not None: if data_extraction_status == "not_started": query = query.filter( Study.data_extraction_status == data_extraction_status - ).filter( - Study.fulltext_status == "included" - ) # this is necessary! + ).filter(Study.fulltext_status == "included") # this is necessary! else: query = query.filter( Study.data_extraction_status == data_extraction_status diff --git a/colandr/apis/resources/study_tags.py b/colandr/apis/resources/study_tags.py index affa9c5a..d53b016a 100644 --- a/colandr/apis/resources/study_tags.py +++ b/colandr/apis/resources/study_tags.py @@ -58,7 +58,10 @@ def get(self, review_id): return not_found_error(f" 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 study tags for this review" diff --git a/colandr/apis/resources/users.py b/colandr/apis/resources/users.py index 6df55133..ac2e1892 100644 --- a/colandr/apis/resources/users.py +++ b/colandr/apis/resources/users.py @@ -61,7 +61,7 @@ def get(self, id, fields): current_user.is_admin is False and id != current_user.id and any( - review.users.filter_by(id=id).one_or_none() + review.review_user_assoc.filter_by(user_id=id).one_or_none() for review in current_user.reviews ) is False @@ -101,7 +101,7 @@ def delete(self, id): return not_found_error(f" not found") db.session.delete(user) db.session.commit() - current_app.logger.info("deleted %s", user) + current_app.logger.info("%s deleted %s", current_user, user) return "", 204 @ns.doc( @@ -145,7 +145,9 @@ def put(self, args, id): setattr(user, key, value) try: db.session.commit() - current_app.logger.info("modified %s", user) + current_app.logger.info( + "%s modified %s, attributes=%s", current_user, user, sorted(args.keys()) + ) except (IntegrityError, InvalidRequestError) as e: current_app.logger.exception("%s: unexpected db error", "UserResource.put") db.session.rollback() @@ -206,7 +208,10 @@ def get(self, email, review_id): return not_found_error(f" 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 see users for this review" diff --git a/colandr/apis/schemas.py b/colandr/apis/schemas.py index a3826149..fef150fc 100644 --- a/colandr/apis/schemas.py +++ b/colandr/apis/schemas.py @@ -32,9 +32,6 @@ class ReviewSchema(Schema): id = fields.Int(dump_only=True) created_at = fields.DateTime(dump_only=True, format="iso") last_updated = fields.DateTime(dump_only=True, format="iso") - owner_user_id = fields.Int( - required=True, validate=Range(min=1, max=constants.MAX_INT) - ) name = fields.Str(required=True, validate=Length(max=500)) description = fields.Str(load_default=None) status = fields.Str(validate=OneOf(constants.REVIEW_STATUSES)) diff --git a/colandr/cli.py b/colandr/cli.py index 241985c1..564b1be3 100644 --- a/colandr/cli.py +++ b/colandr/cli.py @@ -78,14 +78,11 @@ def db_seed(file_path: pathlib.Path): setattr(fulltext, key, val) for record in data["fulltext_screenings"]: db.session.add(models.FulltextScreening(**record)) - # # TODO: figure out why this doesn't work :/ - # for record in seed_data["review_teams"]: - # review = db.session.get(models.Review, record["id"]) - # user = db.session.get(models.User, record["user_id"]) - # if record["action"] == "add": - # review.users.append(user) - # else: - # raise ValueError() + for record in data["review_user_associations"]: + review = db.session.get(models.Review, record["review_id"]) + user = db.session.get(models.User, record["user_id"]) + rua = models.ReviewUserAssoc(review, user, record.get("user_role")) + db.session.add(rua) db.session.commit() diff --git a/colandr/models.py b/colandr/models.py index f6d59530..c7ec517d 100644 --- a/colandr/models.py +++ b/colandr/models.py @@ -1,10 +1,12 @@ import itertools import logging +from typing import Optional import sqlalchemy as sa import werkzeug.security from sqlalchemy import event as sa_event from sqlalchemy.dialects import postgresql +from sqlalchemy.ext.associationproxy import association_proxy from sqlalchemy.ext.hybrid import hybrid_property # from . import tasks # do this once circular import issue gets fixed @@ -15,14 +17,6 @@ LOGGER = logging.getLogger(__name__) -# association table for users-reviews many-to-many relationship -users_reviews = db.Table( - "users_to_reviews", - sa.Column("user_id", sa.Integer, sa.ForeignKey("users.id"), index=True), - sa.Column("review_id", sa.Integer, sa.ForeignKey("reviews.id"), index=True), -) - - class User(db.Model): __tablename__ = "users" @@ -46,12 +40,14 @@ class User(db.Model): is_admin = sa.Column(sa.Boolean, nullable=False, server_default=sa.false()) # relationships - owned_reviews = db.relationship( - "Review", back_populates="owner", lazy="dynamic", passive_deletes=True - ) - reviews = db.relationship( - "Review", secondary=users_reviews, back_populates="users", lazy="dynamic" + review_user_assoc = db.relationship( + "ReviewUserAssoc", + back_populates="user", + cascade="all, delete", + lazy="dynamic", ) + reviews = association_proxy("review_user_assoc", "review") + imports = db.relationship( "Import", back_populates="user", lazy="dynamic", passive_deletes=True ) @@ -68,6 +64,15 @@ class User(db.Model): def __repr__(self): return f"" + @property + def owned_reviews(self) -> list["Review"]: + return [ + rua.review + for rua in db.session.query(ReviewUserAssoc).filter_by( + user_id=self.id, user_role="owner" + ) + ] + @property def password(self): """User's (automatically hashed) password.""" @@ -86,49 +91,6 @@ def hash_password(password: str) -> str: return werkzeug.security.generate_password_hash(password, method="pbkdf2") -class DataSource(db.Model): - __tablename__ = "data_sources" - __table_args__ = ( - db.UniqueConstraint( - "source_type", "source_name", name="source_type_source_name_uc" - ), - ) - - # columns - id = sa.Column(sa.BigInteger, primary_key=True, autoincrement=True) - created_at = sa.Column( - sa.DateTime(timezone=False), - nullable=False, - server_default=sa.text("(CURRENT_TIMESTAMP AT TIME ZONE 'UTC')"), - ) - source_type = sa.Column(sa.String(length=20), nullable=False, index=True) - source_name = sa.Column(sa.String(length=100), index=True) - source_url = sa.Column(sa.String(length=500)) - - @hybrid_property - def source_type_and_name(self): - if self.source_name: - return f"{self.source_type}: {self.source_name}" - else: - return self.source_type - - # relationships - imports = db.relationship( - "Import", back_populates="data_source", lazy="dynamic", passive_deletes=True - ) - studies = db.relationship( - "Study", back_populates="data_source", lazy="dynamic", passive_deletes=True - ) - - def __init__(self, source_type, source_name=None, source_url=None): - self.source_type = source_type - self.source_name = source_name - self.source_url = source_url - - def __repr__(self): - return f"" - - class Review(db.Model): __tablename__ = "reviews" @@ -145,12 +107,6 @@ class Review(db.Model): server_default=sa.text("(CURRENT_TIMESTAMP AT TIME ZONE 'UTC')"), server_onupdate=sa.text("(CURRENT_TIMESTAMP AT TIME ZONE 'UTC')"), ) - owner_user_id = sa.Column( - sa.Integer, - sa.ForeignKey("users.id", ondelete="CASCADE"), - nullable=False, - index=True, - ) name = sa.Column(sa.String(length=500), nullable=False) description = sa.Column(sa.Text) status = sa.Column(sa.String(length=25), server_default="active", nullable=False) @@ -166,15 +122,14 @@ class Review(db.Model): num_fulltexts_excluded = sa.Column(sa.Integer, server_default="0", nullable=False) # relationships - owner = db.relationship( - "User", - foreign_keys=[owner_user_id], - back_populates="owned_reviews", - lazy="select", - ) - users = db.relationship( - "User", secondary=users_reviews, back_populates="reviews", lazy="dynamic" + review_user_assoc = db.relationship( + "ReviewUserAssoc", + back_populates="review", + cascade="all, delete", + lazy="dynamic", ) + users = association_proxy("review_user_assoc", "user") + review_plan = db.relationship( "ReviewPlan", uselist=False, @@ -213,14 +168,56 @@ class Review(db.Model): "DataExtraction", back_populates="review", lazy="dynamic", passive_deletes=True ) - def __init__(self, name, owner_user_id, description=None): + def __init__(self, name, description=None): self.name = name - self.owner_user_id = owner_user_id self.description = description def __repr__(self): return f"" + @property + def owners(self) -> list[User]: + return [ + rua.user + for rua in db.session.query(ReviewUserAssoc).filter_by( + review_id=self.id, user_role="owner" + ) + ] + + +class ReviewUserAssoc(db.Model): + __tablename__ = "review_user_assoc" + + review_id = sa.Column( + sa.Integer, sa.ForeignKey("reviews.id", ondelete="CASCADE"), primary_key=True + ) + user_id = sa.Column( + sa.Integer, sa.ForeignKey("users.id", ondelete="CASCADE"), primary_key=True + ) + user_role = sa.Column(sa.Text, nullable=False, server_default=sa.text("'member'")) + created_at = sa.Column( + sa.DateTime(timezone=False), + nullable=False, + server_default=sa.text("(CURRENT_TIMESTAMP AT TIME ZONE 'UTC')"), + ) + updated_at = sa.Column( + sa.DateTime(timezone=False), + nullable=False, + server_default=sa.text("(CURRENT_TIMESTAMP AT TIME ZONE 'UTC')"), + server_onupdate=sa.text("(CURRENT_TIMESTAMP AT TIME ZONE 'UTC')"), + ) + + review = db.relationship("Review", back_populates="review_user_assoc") + user = db.relationship("User", back_populates="review_user_assoc") + + def __init__(self, review: Review, user: User, user_role: Optional[str] = None): + self.review = review + self.user = user + self.user_role = user_role + + def __repr__(self): + return f"" + class ReviewPlan(db.Model): __tablename__ = "review_plans" @@ -290,6 +287,49 @@ def __repr__(self): return f"" +class DataSource(db.Model): + __tablename__ = "data_sources" + __table_args__ = ( + db.UniqueConstraint( + "source_type", "source_name", name="source_type_source_name_uc" + ), + ) + + # columns + id = sa.Column(sa.BigInteger, primary_key=True, autoincrement=True) + created_at = sa.Column( + sa.DateTime(timezone=False), + nullable=False, + server_default=sa.text("(CURRENT_TIMESTAMP AT TIME ZONE 'UTC')"), + ) + source_type = sa.Column(sa.String(length=20), nullable=False, index=True) + source_name = sa.Column(sa.String(length=100), index=True) + source_url = sa.Column(sa.String(length=500)) + + @hybrid_property + def source_type_and_name(self): + if self.source_name: + return f"{self.source_type}: {self.source_name}" + else: + return self.source_type + + # relationships + imports = db.relationship( + "Import", back_populates="data_source", lazy="dynamic", passive_deletes=True + ) + studies = db.relationship( + "Study", back_populates="data_source", lazy="dynamic", passive_deletes=True + ) + + def __init__(self, source_type, source_name=None, source_url=None): + self.source_type = source_type + self.source_name = source_name + self.source_url = source_url + + def __repr__(self): + return f"" + + class Import(db.Model): __tablename__ = "imports" @@ -1029,21 +1069,21 @@ def update_fulltext_status(mapper, connection, target): data_extraction = connection.execute( sa.select(DataExtraction).where(DataExtraction.id == fulltext_id) ).first() - data_extraction_inserted_or_deleted = False + # data_extraction_inserted_or_deleted = False if status == "included" and data_extraction is None: with connection.begin(): connection.execute( sa.insert(DataExtraction).values(id=fulltext_id, review_id=review_id) ) LOGGER.info("inserted ", fulltext_id) - data_extraction_inserted_or_deleted = True + # data_extraction_inserted_or_deleted = True elif status != "included" and data_extraction is None: with connection.begin(): connection.execute( sa.delete(DataExtraction).where(DataExtraction.id == fulltext_id) ) LOGGER.info("deleted ", fulltext_id) - data_extraction_inserted_or_deleted = True + # data_extraction_inserted_or_deleted = True # we may have to update our counts for review num_fulltexts_included / excluded if old_status != status: if old_status == "included": # decrement num_fulltexts_included diff --git a/migrations/versions/9a941c4cd94e_add_new_review_users_association_table.py b/migrations/versions/9a941c4cd94e_add_new_review_users_association_table.py new file mode 100644 index 00000000..bbe8a48c --- /dev/null +++ b/migrations/versions/9a941c4cd94e_add_new_review_users_association_table.py @@ -0,0 +1,137 @@ +"""add new review users association table + +Revision ID: 9a941c4cd94e +Revises: 10e30b838ff6 +Create Date: 2023-12-05 02:29:15.656167 + +""" +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = "9a941c4cd94e" +down_revision = "10e30b838ff6" +branch_labels = None +depends_on = None + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.create_table( + "review_user_assoc", + sa.Column("review_id", sa.Integer(), nullable=False), + sa.Column("user_id", sa.Integer(), nullable=False), + sa.Column( + "user_role", sa.Text(), server_default=sa.text("'member'"), nullable=False + ), + sa.Column( + "created_at", + sa.DateTime(), + server_default=sa.text("(CURRENT_TIMESTAMP AT TIME ZONE 'UTC')"), + nullable=False, + ), + sa.Column( + "updated_at", + sa.DateTime(), + server_default=sa.text("(CURRENT_TIMESTAMP AT TIME ZONE 'UTC')"), + nullable=False, + ), + sa.ForeignKeyConstraint(["review_id"], ["reviews.id"], ondelete="CASCADE"), + sa.ForeignKeyConstraint(["user_id"], ["users.id"], ondelete="CASCADE"), + sa.PrimaryKeyConstraint("review_id", "user_id"), + ) + + op.execute( + "INSERT INTO review_user_assoc " + "SELECT review_id, user_id FROM users_to_reviews" + ) + op.execute( + """ + MERGE INTO review_user_assoc AS rua + USING reviews AS r + ON rua.review_id = r.id AND rua.user_id = r.owner_user_id + WHEN MATCHED THEN + UPDATE SET user_role = 'owner' + WHEN NOT MATCHED THEN + INSERT (review_id, user_id) + VALUES (r.id, r.owner_user_id) + """ + ) + + with op.batch_alter_table("users_to_reviews", schema=None) as batch_op: + batch_op.drop_index("ix_users_to_reviews_review_id") + batch_op.drop_index("ix_users_to_reviews_user_id") + op.drop_table("users_to_reviews") + + with op.batch_alter_table("reviews", schema=None) as batch_op: + batch_op.drop_index("ix_reviews_owner_user_id") + batch_op.drop_constraint("reviews_owner_user_id_fkey", type_="foreignkey") + batch_op.drop_column("owner_user_id") + + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + with op.batch_alter_table("reviews", schema=None) as batch_op: + batch_op.add_column( + sa.Column( + "owner_user_id", + sa.INTEGER(), + autoincrement=False, + # temporarily allow this to be nullable + nullable=True, + ) + ) + batch_op.create_foreign_key( + "reviews_owner_user_id_fkey", + "users", + ["owner_user_id"], + ["id"], + ondelete="CASCADE", + ) + batch_op.create_index( + "ix_reviews_owner_user_id", ["owner_user_id"], unique=False + ) + + op.create_table( + "users_to_reviews", + sa.Column("user_id", sa.INTEGER(), autoincrement=False, nullable=True), + sa.Column("review_id", sa.INTEGER(), autoincrement=False, nullable=True), + sa.ForeignKeyConstraint( + ["review_id"], ["reviews.id"], name="users_to_reviews_review_id_fkey" + ), + sa.ForeignKeyConstraint( + ["user_id"], ["users.id"], name="users_to_reviews_user_id_fkey" + ), + ) + with op.batch_alter_table("users_to_reviews", schema=None) as batch_op: + batch_op.create_index("ix_users_to_reviews_user_id", ["user_id"], unique=False) + batch_op.create_index( + "ix_users_to_reviews_review_id", ["review_id"], unique=False + ) + + op.execute( + """ + INSERT INTO users_to_reviews + SELECT user_id, review_id FROM review_user_assoc + """ + ) + op.execute( + """ + UPDATE reviews AS r + SET owner_user_id = rua.user_id + FROM review_user_assoc AS rua + WHERE + r.id = rua.review_id + AND rua.user_role = 'owner' + """ + ) + + op.drop_table("review_user_assoc") + + with op.batch_alter_table("reviews", schema=None) as batch_op: + batch_op.alter_column("owner_user_id", nullable=False) + + # ### end Alembic commands ### diff --git a/pyproject.toml b/pyproject.toml index 929da590..e5492917 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -52,27 +52,17 @@ dependencies = [ [project.optional-dependencies] dev = [ - "black~=23.0", "httpx~=0.25.0", "ipython", - "isort~=5.0", # "jupyter", "mypy~=1.0", "pytest~=7.0", - "ruff", + "ruff~=0.1.0", ] [tool.setuptools.packages.find] where = ["colandr"] -[tool.black] -line-length = 88 -target-version = ["py310"] - -[tool.isort] -profile = "black" -lines_after_imports = 2 - [tool.mypy] files = ["colandr/**/*.py"] python_version = "3.10" @@ -87,14 +77,21 @@ addopts = "--verbose" testpaths = ["tests"] [tool.ruff] -select = [ - "E", # pycodestyle rules - "F", # pyflakes rules -] -ignore = ["E501"] +# ignore line-length violations, None comparisons +ignore = ["E501", "E711"] line-length = 88 +indent-width = 4 target-version = "py310" src = ["colandr"] +[tool.ruff.format] +quote-style = "double" +indent-style = "space" +skip-magic-trailing-comma = false +line-ending = "auto" + +[tool.ruff.lint.isort] +lines-after-imports = 2 + [tool.ruff.per-file-ignores] -"__init__.py" = ["F401"] # ignore unused imports in `__init__.py` files +"__init__.py" = ["E402", "F401"] # imports not at top of cell and unused imports diff --git a/requirements/dev.txt b/requirements/dev.txt index 67517bf3..7b621bb2 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -1,9 +1,7 @@ # TODO: keep aligned with pyproject.toml -black~=23.0 ipython httpx~=0.24.0 -isort~=5.0 # jupyter mypy~=1.0 pytest~=7.0 -ruff +ruff~=0.1.0 diff --git a/tests/api/test_review_teams.py b/tests/api/test_review_teams.py new file mode 100644 index 00000000..9d6bdc8c --- /dev/null +++ b/tests/api/test_review_teams.py @@ -0,0 +1,80 @@ +import flask +import pytest + + +class TestReviewTeamResource: + @pytest.mark.parametrize( + ["id_", "params", "status_code"], + [ + (1, None, 200), + (2, None, 200), + (1, {"fields": "id,name"}, 200), + (1, {"fields": "name"}, 200), + (999, None, 404), + ], + ) + def test_get(self, id_, params, status_code, app, client, admin_headers, seed_data): + with app.test_request_context(): + url = flask.url_for( + "review_teams_review_team_resource", id=id_, **(params or {}) + ) + response = client.get(url, headers=admin_headers) + assert response.status_code == status_code + if 200 <= status_code < 300: + data = response.json + seed_data = [ + record + for record in seed_data["review_user_associations"] + if record["review_id"] == id_ + ] + exp_user_ids = sorted(set(record["user_id"] for record in seed_data)) + assert sorted(record["id"] for record in data) == exp_user_ids + exp_is_owners = { + record["user_id"]: record["user_role"] == "owner" + for record in seed_data + } + assert { + record["id"]: record["is_owner"] for record in data + } == exp_is_owners + fields = None if params is None else params["fields"].split(",") + if fields is not None: + for field in ["id", "is_owner"]: + if field not in fields: + fields.append(field) + assert all("id" in record for record in data) + if fields: + assert all(sorted(record.keys()) == sorted(fields) for record in data) + + @pytest.mark.parametrize( + ["id_", "params", "status_code"], + [ + (1, {"action": "make_owner", "user_id": 3}, 200), + (1, {"action": "set_role", "user_id": 3, "user_role": "owner"}, 200), + (1, {"action": "set_role", "user_id": 3, "user_role": "member"}, 200), + (1, {"action": "set_role", "user_id": 4, "user_role": "member"}, 404), + (1, {"action": "remove", "user_id": 3}, 200), + ], + ) + def test_put( + self, id_, params, status_code, app, client, admin_headers, db_session + ): + with app.test_request_context(): + url = flask.url_for("review_teams_review_team_resource", id=id_, **params) + response = client.put(url, json=params, headers=admin_headers) + assert response.status_code == status_code + if 200 <= status_code < 300: + data = response.json + if params["action"] == "make_owner": + modified_user = [ + record for record in data if record["id"] == params["user_id"] + ][0] + assert modified_user["is_owner"] is True + elif params["action"] == "set_role": + modified_user = [ + record for record in data if record["id"] == params["user_id"] + ][0] + assert modified_user["is_owner"] is (params["user_role"] == "owner") + elif params["action"] == "remove": + assert not [ + record for record in data if record["id"] == params["user_id"] + ] diff --git a/tests/fixtures/seed_data.json b/tests/fixtures/seed_data.json index c1e7bfa5..cc90b9c7 100644 --- a/tests/fixtures/seed_data.json +++ b/tests/fixtures/seed_data.json @@ -31,26 +31,39 @@ ], "reviews": [ { - "owner_user_id": 2, "name": "NAME1", "description": "DESCRIPTION1" }, { - "owner_user_id": 3, "name": "NAME2", "description": "DESCRIPTION2" } ], - "review_teams": [ + "review_user_associations": [ { - "id": 1, - "action": "add", - "user_id": 3 + "review_id": 1, + "user_id": 1, + "user_role": "owner" }, { - "id": 2, - "action": "add", - "user_id": 4 + "review_id": 1, + "user_id": 2, + "user_role": "owner" + }, + { + "review_id": 1, + "user_id": 3, + "user_role": "member" + }, + { + "review_id": 2, + "user_id": 2, + "user_role": "owner" + }, + { + "review_id": 2, + "user_id": 3, + "user_role": "member" } ], "review_plans": [