From 8964322224bedef2882ed2109bda3f5f0ad42672 Mon Sep 17 00:00:00 2001 From: Burton DeWilde Date: Thu, 22 Feb 2024 18:28:56 -0500 Subject: [PATCH] Various fixes found in front-end QA (#99) * feat: Add ordering for review-user-assoc * fix: Remove nonexistent order_by method * fix: Use proper sa v2.0 bulk inserts in endpoint * fix: Rename review user assoc attr in api * fix: Fix missing ranker file for relevance order * fix: Allow getting fulltext text content * docs: Fix copy-paste error in docstring * fix: Add docs params for export endpoints * fix: Don't skip first row if not dicts in fileio * fix: Get studies exports working again but not streaming, womp * feat: Allow jwt tokens by user email this is used only in user invitation flows * feat: Update new user invite email template * feat: Allow inviting non-users to review * fix: Fix last couple bugs in invite user flows * fix: First row no longer duped in tabular write * fix: Add user add option for admins only plus a test --- colandr/apis/auth.py | 37 +++++++++++--- colandr/apis/resources/citation_imports.py | 21 ++++---- colandr/apis/resources/citation_screenings.py | 6 +-- colandr/apis/resources/citations.py | 2 +- colandr/apis/resources/data_extractions.py | 12 +++-- colandr/apis/resources/deduplicate_studies.py | 2 +- colandr/apis/resources/exports.py | 44 +++++++++++++---- colandr/apis/resources/fulltext_uploads.py | 4 +- colandr/apis/resources/review_teams.py | 49 ++++++++++--------- colandr/apis/resources/reviews.py | 2 +- colandr/apis/resources/studies.py | 17 +++---- colandr/apis/schemas.py | 1 + colandr/lib/fileio/tabular.py | 7 +-- colandr/models.py | 2 + .../emails/invite_new_user_to_review.html | 12 +++-- tests/api/test_review_teams.py | 1 + 16 files changed, 137 insertions(+), 82 deletions(-) diff --git a/colandr/apis/auth.py b/colandr/apis/auth.py index 3f3f5542..1b94b4a0 100644 --- a/colandr/apis/auth.py +++ b/colandr/apis/auth.py @@ -308,13 +308,17 @@ def put(self, args, token): @jwt.user_identity_loader -def user_identity_loader(user: User): +def user_identity_loader(user: t.Union[User, str]): """ Callback function that takes the ``User`` passed in as the "identity" when creating JWTs and returns it in JSON serializable format, i.e. as the corresponding unique integer ``User.id`` . """ - return user.id + if isinstance(user, User): + return user.id + elif isinstance(user, str): + Email()(user) # validate as email + return user @jwt.user_lookup_loader @@ -324,14 +328,25 @@ def user_lookup_callback(_jwt_header, jwt_data: dict) -> User: whenever a protected API route is accessed. """ identity = jwt_data[current_app.config["JWT_IDENTITY_CLAIM"]] - user = db.session.get(User, identity) + if isinstance(identity, int): # id + user = db.session.get(User, identity) + elif isinstance(identity, str): # email + Email()(identity) # validate as email + user = db.session.execute( + sa.select(User).filter_by(email=identity) + ).scalar_one_or_none() + else: + raise TypeError() assert user is not None # type guard return user @jwt.additional_claims_loader -def additional_claims_loader(user: User) -> dict: - return {"is_admin": user.is_admin} +def additional_claims_loader(user: t.Union[User, str]) -> dict: + if isinstance(user, User): + return {"is_admin": user.is_admin} + else: + return {} @jwt.token_in_blocklist_loader @@ -379,8 +394,16 @@ def get_user_from_token(token: str) -> t.Optional[User]: if it exists in the database; otherwise, return None. """ jwt_data = jwtext.decode_token(token, allow_expired=False) - user_id = jwt_data[current_app.config["JWT_IDENTITY_CLAIM"]] - user = db.session.get(User, user_id) + identity = jwt_data[current_app.config["JWT_IDENTITY_CLAIM"]] + if isinstance(identity, int): + user = db.session.get(User, identity) + elif isinstance(identity, str): + Email()(identity) # validate as email + user = db.session.execute( + sa.select(User).filter_by(email=identity) + ).scalar_one_or_none() + else: + raise TypeError() return user diff --git a/colandr/apis/resources/citation_imports.py b/colandr/apis/resources/citation_imports.py index 3e36c35c..0ac7871e 100644 --- a/colandr/apis/resources/citation_imports.py +++ b/colandr/apis/resources/citation_imports.py @@ -227,10 +227,11 @@ def post( ] # insert studies, and get their primary keys _back_ - stmt = sa.insert(Study).values(studies_to_insert).returning(Study.id) - with db.engine.connect() as conn: - study_ids = [result[0] for result in conn.execute(stmt)] - + study_ids = list( + db.session.execute( + sa.insert(Study).returning(Study.id), studies_to_insert + ).scalars() + ) # add study ids to citations as their primary keys # then bulk insert as mappings # this method is required because not all citations have all fields @@ -242,14 +243,10 @@ def post( # the corresponding fulltexts, since bulk operations won't trigger # the fancy events defined in models.py if status == "included": - with db.engine.connect() as conn: - conn.execute( - Fulltext.__table__.insert(), - [ - {"id": study_id, "review_id": review_id} - for study_id in study_ids - ], - ) + db.session.execute( + sa.insert(Fulltext), + [{"id": study_id, "review_id": review_id} for study_id in study_ids], + ) # don't forget about a record of the import citations_import = Import( diff --git a/colandr/apis/resources/citation_screenings.py b/colandr/apis/resources/citation_screenings.py index 8e12161e..d2c3dae0 100644 --- a/colandr/apis/resources/citation_screenings.py +++ b/colandr/apis/resources/citation_screenings.py @@ -68,7 +68,7 @@ def get(self, id, fields): return not_found_error(f" not found") if ( current_user.is_admin is False - and current_user.user_review_assoc.filter_by( + and current_user.review_user_assoc.filter_by( review_id=citation.review_id ).one_or_none() is None @@ -104,7 +104,7 @@ def delete(self, id): return not_found_error(f" not found") if ( current_user.is_admin is False - and current_user.user_review_assoc.filter_by( + and current_user.review_user_assoc.filter_by( review_id=citation.review_id ).one_or_none() is None @@ -150,7 +150,7 @@ def post(self, args, id): return not_found_error(f" not found") if ( current_user.is_admin is False - and current_user.user_review_assoc.filter_by( + and current_user.review_user_assoc.filter_by( review_id=citation.review_id ).one_or_none() is None diff --git a/colandr/apis/resources/citations.py b/colandr/apis/resources/citations.py index d0e1d6ac..30b4cf34 100644 --- a/colandr/apis/resources/citations.py +++ b/colandr/apis/resources/citations.py @@ -225,7 +225,7 @@ 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.user_review_assoc.filter_by( + and current_user.review_user_assoc.filter_by( review_id=review_id ).one_or_none() is None diff --git a/colandr/apis/resources/data_extractions.py b/colandr/apis/resources/data_extractions.py index f9d6db16..fd56113f 100644 --- a/colandr/apis/resources/data_extractions.py +++ b/colandr/apis/resources/data_extractions.py @@ -52,10 +52,14 @@ def get(self, id): extracted_data = db.session.get(DataExtraction, id) if not extracted_data: return not_found_error(f" not found") + # TODO: figure out if this is "better" approach + # if current_user.is_admin is False and not any( + # review.id == extracted_data.review_id for review in current_user.reviews + # ): if ( current_user.is_admin is False - and current_user.reviews.filter_by( - id=extracted_data.review_id + and current_user.review_user_assoc.filter_by( + review_id=extracted_data.review_id ).one_or_none() is None ): @@ -103,7 +107,7 @@ def delete(self, id, labels): if not extracted_data: return not_found_error(f" not found") if ( - current_user.user_review_assoc.filter_by( + current_user.review_user_assoc.filter_by( review_id=extracted_data.review_id ).one_or_none() is None @@ -156,7 +160,7 @@ def put(self, args, id): if not extracted_data: return not_found_error(f" not found") if ( - current_user.user_review_assoc.filter_by(review_id=review_id).one_or_none() + current_user.review_user_assoc.filter_by(review_id=review_id).one_or_none() is None ): return forbidden_error( diff --git a/colandr/apis/resources/deduplicate_studies.py b/colandr/apis/resources/deduplicate_studies.py index 6b9df9b3..3c62eb91 100644 --- a/colandr/apis/resources/deduplicate_studies.py +++ b/colandr/apis/resources/deduplicate_studies.py @@ -47,7 +47,7 @@ class DeduplicateStudiesResource(Resource): ) @jwtext.jwt_required() def post(self, review_id): - """get all distinct tags assigned to studies""" + """manually trigger deduplication of studies for a given review""" current_user = jwtext.get_current_user() review = db.session.get(models.Review, review_id) if not review: diff --git a/colandr/apis/resources/exports.py b/colandr/apis/resources/exports.py index 899caaaa..d561a2fb 100644 --- a/colandr/apis/resources/exports.py +++ b/colandr/apis/resources/exports.py @@ -24,6 +24,16 @@ class ExportStudiesResource(Resource): @ns.doc( description="export studies data", + params={ + "review_id": { + "in": "query", + "type": "integer", + }, + "content_type": { + "in": "query", + "type": "string", + }, + }, responses={ 200: "successfully got studies data for specified review", 403: "current app user forbidden to export studies data for specified review", @@ -56,13 +66,6 @@ def get(self, review_id, content_type): ): return forbidden_error(f"{current_user} forbidden to get this review") - studies = db.session.execute( - sa.select(Study).filter_by(review_id=review_id).order_by(Study.id) - ).scalars() - data_extraction_form = db.session.execute( - sa.select(ReviewPlan.data_extraction_form).filter_by(id=review_id) - ).one_or_none() - fieldnames = [ "study_id", "study_tags", @@ -85,6 +88,9 @@ def get(self, review_id, content_type): "fulltext_exclude_reasons", ] extraction_label_types: t.Optional[list[tuple[str, str]]] + data_extraction_form = db.session.execute( + sa.select(ReviewPlan.data_extraction_form).filter_by(id=review_id) + ).one_or_none() if data_extraction_form: extraction_label_types = [ (item["label"], item["field_type"]) for item in data_extraction_form[0] @@ -93,7 +99,15 @@ def get(self, review_id, content_type): else: extraction_label_types = None - rows = (_study_to_row(study, extraction_label_types) for study in studies) + # TODO: make this query performant and fully streamable, even with lazy-loading + # see: https://docs.sqlalchemy.org/en/14/errors.html#parent-instance-x-is-not-bound-to-a-session-lazy-load-deferred-load-refresh-etc-operation-cannot-proceed + # see: https://docs.sqlalchemy.org/en/14/errors.html#object-cannot-be-converted-to-persistent-state-as-this-identity-map-is-no-longer-valid + studies = db.session.execute( + sa.select(Study).filter_by(review_id=review_id).order_by(Study.id), + execution_options={"prebuffer_rows": True}, + ).scalars() + # rows = (_study_to_row(study, extraction_label_types) for study in studies) + rows = [_study_to_row(study, extraction_label_types) for study in studies] if content_type == "text/csv": export_data = fileio.tabular.write_stream( fieldnames, rows, quoting=csv.QUOTE_NONNUMERIC @@ -102,9 +116,13 @@ def get(self, review_id, content_type): raise NotImplementedError("only 'text/csv' content type is available") response = make_response(export_data, 200) - response.headers["Content-type"] = content_type + response.headers.update( + { + "Content-Type": content_type, + "Content-Disposition": "attachment; filename=colandr-review-studies.csv", + } + ) current_app.logger.info("studies data exported for %s", review) - return response @@ -179,6 +197,12 @@ def _study_to_row( class ExportScreeningsResource(Resource): @ns.doc( description="export screenings data", + params={ + "review_id": { + "in": "query", + "type": "integer", + }, + }, responses={ 200: "successfully got screenings data for specified review", 403: "current app user forbidden to export screenings data for specified review", diff --git a/colandr/apis/resources/fulltext_uploads.py b/colandr/apis/resources/fulltext_uploads.py index 8bf712be..3279176e 100644 --- a/colandr/apis/resources/fulltext_uploads.py +++ b/colandr/apis/resources/fulltext_uploads.py @@ -143,7 +143,7 @@ def post(self, id, uploaded_file): return not_found_error(f" not found") if ( current_user.is_admin is False - and current_user.user_review_assoc.filter_by( + and current_user.review_user_assoc.filter_by( review_id=fulltext.review_id ).one_or_none() is None @@ -221,7 +221,7 @@ def delete(self, id): return not_found_error(f" not found") if ( current_user.is_admin is False - and current_user.user_review_assoc.filter_by( + and current_user.review_user_assoc.filter_by( review_id=fulltext.review_id ).one_or_none() is None diff --git a/colandr/apis/resources/review_teams.py b/colandr/apis/resources/review_teams.py index 0db7699a..8842bbc5 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 @@ -170,29 +170,33 @@ def put(self, id, action, user_id, user_email, user_role): elif user in review_users: return forbidden_error(f"{user} is already on this review") else: - review_users.append(user) + review.review_user_assoc.append(ReviewUserAssoc(review, user)) # user is being *invited*, so send an invitation email elif action == "invite": - if user is None: - return not_found_error("no user found with given id or email") + if user is not None: + identity = user + user_email = user.email + template_name = "emails/invite_user_to_review.html" else: - token = jwtext.create_access_token(identity=user) - confirm_url = flask.url_for( - "review_teams_confirm_review_team_invite_resource", - id=id, - token=token, - _external=True, - ) - html = render_template( - "emails/invite_user_to_review.html", - url=confirm_url, - inviter_email=current_user.email, - review_name=review.name, + identity = user_email + template_name = "emails/invite_new_user_to_review.html" + token = jwtext.create_access_token(identity=identity) + confirm_url = flask.url_for( + "review_teams_confirm_review_team_invite_resource", + id=id, + token=token, + _external=True, + ) + html = render_template( + template_name, + url=confirm_url, + inviter_email=current_user.email, + review_name=review.name, + ) + if current_app.config["MAIL_SERVER"]: + tasks.send_email.apply_async( + args=[[user_email], "Let's collaborate!", "", html] ) - if current_app.config["MAIL_SERVER"]: - tasks.send_email.apply_async( - args=[[user.email], "Let's collaborate!", "", html] - ) elif action in ("make_owner", "set_role"): if user is None: return not_found_error("no user found with given id or email") @@ -255,14 +259,13 @@ def get(self, id, token): review = db.session.get(Review, id) if not review: return not_found_error(f" not found") - review_users = review.users user = auth.get_user_from_token(token) if user is None: return not_found_error(f"no user found for token='{token}'") - if user not in review_users: - review_users.append(user) + if user not in review.users: + db.session.add(ReviewUserAssoc(review, user)) else: return forbidden_error(f"{user} is already on this review") diff --git a/colandr/apis/resources/reviews.py b/colandr/apis/resources/reviews.py index 305272de..bf146d1f 100644 --- a/colandr/apis/resources/reviews.py +++ b/colandr/apis/resources/reviews.py @@ -196,7 +196,7 @@ def get(self, fields, _review_ids): f'non-admin {current_user} passed admin-only "_review_ids" param' ) else: - reviews = current_user.reviews.order_by(Review.id).all() + reviews = current_user.reviews if fields and "id" not in fields: fields.append("id") return ReviewSchema(only=fields, many=True).dump(reviews) diff --git a/colandr/apis/resources/studies.py b/colandr/apis/resources/studies.py index dc93d88f..807741d4 100644 --- a/colandr/apis/resources/studies.py +++ b/colandr/apis/resources/studies.py @@ -297,7 +297,7 @@ def get( return not_found_error(f" not found") if ( current_user.is_admin is False - and current_user.user_review_assoc.filter_by( + and current_user.review_user_assoc.filter_by( review_id=review_id ).one_or_none() is None @@ -431,18 +431,15 @@ def get( scores = None # best option: we have a trained citation ranking model - try: - ranker = Ranker.load( - os.path.join( - current_app.config["RANKING_MODELS_DIR"], str(review_id) - ), - review_id, - ) + ranker = Ranker.load( + os.path.join(current_app.config["RANKING_MODELS_DIR"], str(review_id)), + review_id, + ) + if os.path.exists(str(ranker.model_fpath)): + # ranker model available :) scores = ranker.predict( result.citation.text_content_vector_rep for result in results ) - except FileNotFoundError: - pass # no ranker model available :/ # next best option: both positive and negative keyterms if not scores: diff --git a/colandr/apis/schemas.py b/colandr/apis/schemas.py index aeb61401..ef1bb54d 100644 --- a/colandr/apis/schemas.py +++ b/colandr/apis/schemas.py @@ -181,6 +181,7 @@ class FulltextSchema(Schema): review_id = fields.Int(required=True, validate=Range(min=1, max=constants.MAX_INT)) filename = fields.Str(validate=Length(max=30)) original_filename = fields.Str(dump_only=True) + text_content = fields.Str(dump_only=True) screenings = fields.Nested(ScreeningSchema, many=True, dump_only=True) diff --git a/colandr/lib/fileio/tabular.py b/colandr/lib/fileio/tabular.py index 6e8fd8e4..990d5fae 100644 --- a/colandr/lib/fileio/tabular.py +++ b/colandr/lib/fileio/tabular.py @@ -35,8 +35,9 @@ def write_stream( Reference: https://stackoverflow.com/questions/32608265/streaming-a-generated-csv-with-flask """ - first_row = next(iter(rows)) - rows_ = itertools.chain([first_row], rows) + iter_rows = iter(rows) + first_row = next(iter_rows) + rows_ = itertools.chain((first_row,), iter_rows) if isinstance(first_row, dict): writer = csv.DictWriter(DummyWriter(), cols, dialect=dialect, **kwargs) yield writer.writeheader() @@ -45,7 +46,7 @@ def write_stream( else: writer = csv.writer(DummyWriter(), dialect, **kwargs) yield writer.writerow(cols) - for row in rows: + for row in rows_: yield writer.writerow(row) diff --git a/colandr/models.py b/colandr/models.py index b0ba7502..c8a5538a 100644 --- a/colandr/models.py +++ b/colandr/models.py @@ -51,6 +51,7 @@ class User(db.Model): back_populates="user", cascade="all, delete", lazy="dynamic", + order_by="ReviewUserAssoc.review_id", ) reviews = association_proxy("review_user_assoc", "review") @@ -132,6 +133,7 @@ class Review(db.Model): back_populates="review", cascade="all, delete", lazy="dynamic", + order_by="ReviewUserAssoc.user_id", ) users = association_proxy("review_user_assoc", "user") diff --git a/colandr/templates/emails/invite_new_user_to_review.html b/colandr/templates/emails/invite_new_user_to_review.html index 5d524750..d656dfbe 100644 --- a/colandr/templates/emails/invite_new_user_to_review.html +++ b/colandr/templates/emails/invite_new_user_to_review.html @@ -1,7 +1,9 @@ -

Hey,

-

You've been invited by {{ inviter_email }} to work together in colandr on a systematic review, "{{ review_name }}".

-

But, first things first: You'll need to create an account with us. Please sign yourself up at colandrapp.com, and confirm your account via email.

+

Hi!

+

You've been invited by {{ inviter_email }} to work together in colandr on a systematic review, "{{ review_name + }}".

+

But first you'll need to create an account with us. Please sign yourself up at colandrapp.com, and confirm your account via email.

Then click the link below to join the review:

-

Great, let's collaborate!

+

Let's collaborate!


-

Cheers,
The Colandr Team

+

Cheers,
The Colandr Team

diff --git a/tests/api/test_review_teams.py b/tests/api/test_review_teams.py index 6305ded5..17e6855c 100644 --- a/tests/api/test_review_teams.py +++ b/tests/api/test_review_teams.py @@ -54,6 +54,7 @@ def test_get(self, id_, params, status_code, app, client, admin_headers, seed_da (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), + (1, {"action": "add", "user_id": 4}, 200), ], ) def test_put(self, id_, params, status_code, app, client, admin_headers):