From a02956d4a8751212206d8aa17eead1e33948a735 Mon Sep 17 00:00:00 2001 From: Luke Granger-Brown Date: Sun, 4 Feb 2024 17:11:17 +0000 Subject: [PATCH 1/4] Create ProposalOrdering, to maintain "happens-before" relationship of pairs of Proposals. --- .../8dac53c38bde_create_proposalordering.py | 91 +++++++++++++++++++ models/cfp.py | 32 +++++++ 2 files changed, 123 insertions(+) create mode 100644 migrations/versions/8dac53c38bde_create_proposalordering.py diff --git a/migrations/versions/8dac53c38bde_create_proposalordering.py b/migrations/versions/8dac53c38bde_create_proposalordering.py new file mode 100644 index 000000000..6faef59c5 --- /dev/null +++ b/migrations/versions/8dac53c38bde_create_proposalordering.py @@ -0,0 +1,91 @@ +"""Create ProposalOrdering + +Revision ID: 8dac53c38bde +Revises: 5e48dc411113 +Create Date: 2024-02-04 17:28:55.400686 + +""" + +# revision identifiers, used by Alembic. +revision = "8dac53c38bde" +down_revision = "5e48dc411113" + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.create_table( + "proposal_ordering_version", + sa.Column( + "happens_first_proposal_id", + sa.Integer(), + autoincrement=False, + nullable=False, + ), + sa.Column( + "happens_later_proposal_id", + sa.Integer(), + autoincrement=False, + nullable=False, + ), + sa.Column( + "transaction_id", sa.BigInteger(), autoincrement=False, nullable=False + ), + sa.Column("operation_type", sa.SmallInteger(), nullable=False), + sa.PrimaryKeyConstraint( + "happens_first_proposal_id", + "happens_later_proposal_id", + "transaction_id", + name=op.f("pk_proposal_ordering_version"), + ), + ) + op.create_index( + op.f("ix_proposal_ordering_version_operation_type"), + "proposal_ordering_version", + ["operation_type"], + unique=False, + ) + op.create_index( + op.f("ix_proposal_ordering_version_transaction_id"), + "proposal_ordering_version", + ["transaction_id"], + unique=False, + ) + op.create_table( + "proposal_ordering", + sa.Column("happens_first_proposal_id", sa.Integer(), nullable=False), + sa.Column("happens_later_proposal_id", sa.Integer(), nullable=False), + sa.ForeignKeyConstraint( + ["happens_first_proposal_id"], + ["proposal.id"], + name=op.f("fk_proposal_ordering_happens_first_proposal_id_proposal"), + ), + sa.ForeignKeyConstraint( + ["happens_later_proposal_id"], + ["proposal.id"], + name=op.f("fk_proposal_ordering_happens_later_proposal_id_proposal"), + ), + sa.PrimaryKeyConstraint( + "happens_first_proposal_id", + "happens_later_proposal_id", + name=op.f("pk_proposal_ordering"), + ), + ) + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_table("proposal_ordering") + op.drop_index( + op.f("ix_proposal_ordering_version_transaction_id"), + table_name="proposal_ordering_version", + ) + op.drop_index( + op.f("ix_proposal_ordering_version_operation_type"), + table_name="proposal_ordering_version", + ) + op.drop_table("proposal_ordering_version") + # ### end Alembic commands ### diff --git a/models/cfp.py b/models/cfp.py index 8126d716d..ef7ec4f01 100644 --- a/models/cfp.py +++ b/models/cfp.py @@ -348,6 +348,23 @@ class InvalidVenueException(Exception): ), ) +ProposalOrdering = db.Table( + "proposal_ordering", + BaseModel.metadata, + db.Column( + "happens_first_proposal_id", + db.Integer, + db.ForeignKey("proposal.id"), + primary_key=True, + ), + db.Column( + "happens_later_proposal_id", + db.Integer, + db.ForeignKey("proposal.id"), + primary_key=True, + ), +) + class Proposal(BaseModel): __versioned__ = {"exclude": ["favourites", "favourite_count"]} @@ -394,6 +411,21 @@ class Proposal(BaseModel): favourites = db.relationship( User, secondary=FavouriteProposal, backref=db.backref("favourites") ) + happens_after = db.relationship( + "Proposal", + secondary=ProposalOrdering, + back_populates="happens_before", + primaryjoin=lambda: ProposalOrdering.c.happens_later_proposal_id == Proposal.id, + secondaryjoin=lambda: Proposal.id + == ProposalOrdering.c.happens_first_proposal_id, + ) + happens_before = db.relationship( + "Proposal", + secondary=ProposalOrdering, + back_populates="happens_after", + primaryjoin=lambda: ProposalOrdering.c.happens_first_proposal_id == Proposal.id, + secondaryjoin=lambda: Proposal.id == ProposalOrdering.c.happens_later_proposal_id, + ) # Convenience for individual objects. Use an outerjoin and groupby for more than a few records favourite_count = column_property( From 4a63624daaf6a3717a83272ef19f48a6f1b38acd Mon Sep 17 00:00:00 2001 From: Luke Granger-Brown Date: Sun, 4 Feb 2024 17:36:35 +0000 Subject: [PATCH 2/4] Add dependency order validation, to prevent loops or things depending upon themselves, somehow. --- models/cfp.py | 28 +++++++++++++++++++++++++++- poetry.lock | 24 +++++++++++++++++++++++- pyproject.toml | 2 ++ 3 files changed, 52 insertions(+), 2 deletions(-) diff --git a/models/cfp.py b/models/cfp.py index ef7ec4f01..b6e82ba2e 100644 --- a/models/cfp.py +++ b/models/cfp.py @@ -6,6 +6,7 @@ from itertools import groupby from geoalchemy2 import Geometry from geoalchemy2.shape import to_shape +from toposort import toposort, CircularDependencyError from sqlalchemy import UniqueConstraint, func, select from sqlalchemy.orm import column_property @@ -424,7 +425,8 @@ class Proposal(BaseModel): secondary=ProposalOrdering, back_populates="happens_after", primaryjoin=lambda: ProposalOrdering.c.happens_first_proposal_id == Proposal.id, - secondaryjoin=lambda: Proposal.id == ProposalOrdering.c.happens_later_proposal_id, + secondaryjoin=lambda: Proposal.id + == ProposalOrdering.c.happens_later_proposal_id, ) # Convenience for individual objects. Use an outerjoin and groupby for more than a few records @@ -784,6 +786,30 @@ def get_conflicting_content(self) -> list["Proposal"]: if p.overlaps_with(self) ] + def find_happens_before_causality_violations(self) -> str | None: + """Ensure that there are no loops in happens-before relationships. + + This is done by attempting to perform a toposort on the relationships. + + This actually validates the entire relationship set, rather than just the + relationships that involve the current proposal. + + Returns a string that attempts to explain the violation if one is found. + """ + # Construct a mapping of Proposal ID -> things that happen before it + relationships: dict[int, set[int]] = defaultdict(set) + for happens_first, happens_later in db.session.query(ProposalOrdering).all(): + relationships[happens_later].add(happens_first) + if happens_first == happens_later: + return str(f"Proposal {happens_first} depends upon itself") + + try: + list(toposort(relationships)) + except CircularDependencyError as e: + return str(e) + + return None + @property def start_date(self): return self.scheduled_time diff --git a/poetry.lock b/poetry.lock index fdda1c40d..fe1940758 100644 --- a/poetry.lock +++ b/poetry.lock @@ -3729,6 +3729,17 @@ webencodings = ">=0.4" doc = ["sphinx", "sphinx_rtd_theme"] test = ["flake8", "isort", "pytest"] +[[package]] +name = "toposort" +version = "1.10" +description = "Implements a topological sort algorithm." +optional = false +python-versions = "*" +files = [ + {file = "toposort-1.10-py3-none-any.whl", hash = "sha256:cbdbc0d0bee4d2695ab2ceec97fe0679e9c10eab4b2a87a9372b929e70563a87"}, + {file = "toposort-1.10.tar.gz", hash = "sha256:bfbb479c53d0a696ea7402601f4e693c97b0367837c8898bc6471adfca37a6bd"}, +] + [[package]] name = "tqdm" version = "4.66.1" @@ -3844,6 +3855,17 @@ files = [ {file = "types_simplejson-3.19.0.2-py3-none-any.whl", hash = "sha256:8ba093dc7884f59b3e62aed217144085e675a269debc32678fd80e0b43b2b86f"}, ] +[[package]] +name = "types-toposort" +version = "1.10.0.1" +description = "Typing stubs for toposort" +optional = false +python-versions = "*" +files = [ + {file = "types-toposort-1.10.0.1.tar.gz", hash = "sha256:7e4650ac5a0e5854bf4cd44b9c79ed5914731472ce5e1ac2074fafd8c691073b"}, + {file = "types_toposort-1.10.0.1-py3-none-any.whl", hash = "sha256:0c9ab5789db82c03e6ffb1e6f627d4f83ab4d28f4dee69b1b70eaf65f646021b"}, +] + [[package]] name = "types-urllib3" version = "1.26.25.14" @@ -4316,4 +4338,4 @@ testing = ["coverage (>=5.0.3)", "zope.event", "zope.testing"] [metadata] lock-version = "2.0" python-versions = "~3.11" -content-hash = "f78a2d4b8556b815794afc47a6a4736a8c715b1c4621ea33c6f1d1bc15d940f0" +content-hash = "e1995c5d822df48e9569abb0975f6b920bba8ab92f2b842fd1d7a787046b920e" diff --git a/pyproject.toml b/pyproject.toml index 24b81d91f..08b2dcc6b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -56,6 +56,8 @@ pywisetransfer = "^0.3.1" freezegun = "^1.1.0" logging_tree = "^1.9" flask-mailman = "^0.3.0" +toposort = "^1.10" +types-toposort = "^1.10.0.1" [tool.poetry.group.dev.dependencies] From 0c2d7e97ede3612f7919baa8c79e132043516225 Mon Sep 17 00:00:00 2001 From: Luke Granger-Brown Date: Sun, 4 Feb 2024 19:14:02 +0000 Subject: [PATCH 3/4] Add CfP Review UI for creating ordering relationships. I'm so sorry. --- apps/cfp_review/base.py | 113 ++++++++++++++++-- apps/cfp_review/forms.py | 12 ++ .../cfp_review/_proposals_filter_form.html | 3 +- .../cfp_review/add_happens_relationship.html | 74 ++++++++++++ templates/cfp_review/update_proposal.html | 61 +++++++++- 5 files changed, 253 insertions(+), 10 deletions(-) create mode 100644 templates/cfp_review/add_happens_relationship.html diff --git a/apps/cfp_review/base.py b/apps/cfp_review/base.py index 59b2f20a0..9e3c26b92 100644 --- a/apps/cfp_review/base.py +++ b/apps/cfp_review/base.py @@ -1,6 +1,7 @@ from datetime import timedelta from collections import defaultdict, Counter from itertools import combinations +from werkzeug.exceptions import BadRequest import dateutil from flask import ( @@ -56,6 +57,8 @@ AddNoteForm, ChangeProposalOwner, ReversionForm, + AddOrderingForm, + RemoveOrderingForm, ) from . import ( cfp_review, @@ -101,12 +104,14 @@ def bool_qs(val): raise ValueError("Invalid querystring boolean") -def filter_proposal_request(): +def filter_proposal_request(base_query=None): + if base_query is None: + base_query = Proposal.query bool_names = ["one_day", "needs_help", "needs_money"] bool_vals = [request.args.get(n, type=bool_qs) for n in bool_names] bool_dict = {n: v for n, v in zip(bool_names, bool_vals) if v is not None} - proposal_query = Proposal.query.filter_by(**bool_dict) + proposal_query = base_query.filter_by(**bool_dict) filtered = False @@ -122,7 +127,6 @@ def filter_proposal_request(): show_user_scheduled = request.args.get("show_user_scheduled", type=bool_qs) if show_user_scheduled is None or show_user_scheduled is False: - filtered = False proposal_query = proposal_query.filter_by(user_scheduled=False) # This block has to be last because it will join to the user table @@ -869,7 +873,7 @@ def close_round(): if form.validate_on_submit(): if form.confirm.data: min_votes = session["min_votes"] - for (prop, vote_count) in proposals: + for prop, vote_count in proposals: if vote_count >= min_votes and prop.state != "reviewed": prop.set_state("reviewed") @@ -896,7 +900,7 @@ def close_round(): # Find proposals where the submitter has already had an accepted proposal # or another proposal in this list duplicates = {} - for (prop, _) in proposals: + for prop, _ in proposals: if prop.user.proposals.count() > 1: # Only add each proposal once if prop.user not in duplicates: @@ -937,8 +941,7 @@ def rank(): if form.confirm.data: min_score = session["min_score"] count = 0 - for (prop, score) in scored_proposals: - + for prop, score in scored_proposals: if score >= min_score: count += 1 prop.set_state("accepted") @@ -1168,7 +1171,7 @@ def clashfinder(): clashes = [] offset = 0 - for ((id1, id2), count) in popularity.most_common()[:1000]: + for (id1, id2), count in popularity.most_common()[:1000]: offset += 1 prop1 = Proposal.query.get(id1) prop2 = Proposal.query.get(id2) @@ -1232,4 +1235,98 @@ def proposals_summary(): ) +@cfp_review.route("/proposals/add-happens-relationship", methods=["GET", "POST"]) +@admin_required +def add_happens_relationship(): + form = AddOrderingForm() + if form.validate_on_submit(): + happens_first = Proposal.query.get_or_404(form.happens_first_id.data) + happens_later = Proposal.query.get_or_404(form.happens_later_id.data) + happens_first.happens_before.append(happens_later) + if violations := happens_first.find_happens_before_causality_violations(): + flash(f"Found a causality violation: {violations}") + else: + db.session.add(happens_first) + db.session.commit() + flash( + f"Added happens-before relationship between {happens_first.title} and {happens_later.title}.", + ) + return redirect(request.args.get("next")) + + if "happens_first_id" in request.args: + proposal = Proposal.query.get_or_404(request.args["happens_first_id"]) + direction = "happen after" + + def generate_form(candidate): + form = AddOrderingForm() + form.happens_first_id.data = proposal.id + form.happens_later_id.data = candidate.id + return form + + elif "happens_later_id" in request.args: + proposal = Proposal.query.get_or_404(request.args["happens_later_id"]) + direction = "happen before" + + def generate_form(candidate): + form = AddOrderingForm() + form.happens_first_id.data = candidate.id + form.happens_later_id.data = proposal.id + return form + + else: + raise BadRequest("No proposal ID specified") + + base_qs = { + k: v + for k, v in request.args.items() + if k in ["happens_first_id", "happens_later_id", "next"] + } + + base_query = Proposal.query.filter(Proposal.id != proposal.id) + proposals, filtered = filter_proposal_request(base_query=base_query) + non_sort_query_string = copy_request_args(request.args) + + if "sort_by" in non_sort_query_string: + del non_sort_query_string["sort_by"] + + if "reverse" in non_sort_query_string: + del non_sort_query_string["reverse"] + + tag_counts = {t.tag: [0, len(t.proposals)] for t in Tag.query.all()} + for prop in proposals: + for t in prop.tags: + tag_counts[t.tag][0] = tag_counts[t.tag][0] + 1 + + return render_template( + "cfp_review/add_happens_relationship.html", + proposal=proposal, + generate_form=generate_form, + proposals=proposals, + base_qs=base_qs, + new_qs=non_sort_query_string, + filtered=filtered, + total_proposals=base_query.count(), + tag_counts=tag_counts, + direction=direction, + ) + + +@cfp_review.route("/proposals/remove-happens-relationship", methods=["POST"]) +@admin_required +def remove_happens_relationship(): + form = RemoveOrderingForm() + + if form.validate_on_submit(): + happens_first = Proposal.query.get_or_404(form.happens_first_id.data) + happens_later = Proposal.query.get_or_404(form.happens_later_id.data) + happens_first.happens_before.remove(happens_later) + db.session.add(happens_first) + db.session.commit() + flash( + f"Removed happens-before relationship between {happens_first.title} and {happens_later.title}." + ) + + return redirect(form.next.data) + + from . import venues # noqa diff --git a/apps/cfp_review/forms.py b/apps/cfp_review/forms.py index ed1f79015..1a6e06b42 100644 --- a/apps/cfp_review/forms.py +++ b/apps/cfp_review/forms.py @@ -410,3 +410,15 @@ class ReversionForm(Form): proposal_id = HiddenIntegerField("Proposal ID") txn_id = HiddenIntegerField("Transaction ID") revert = SubmitField("Revert to this version") + + +class RemoveOrderingForm(Form): + happens_first_id = HiddenIntegerField("Happens First") + happens_later_id = HiddenIntegerField("Happens Later") + next = StringField("Return to URL") + + +class AddOrderingForm(Form): + happens_first_id = HiddenIntegerField("Happens First") + happens_later_id = HiddenIntegerField("Happens Later") + submit = SubmitField("Select") diff --git a/templates/cfp_review/_proposals_filter_form.html b/templates/cfp_review/_proposals_filter_form.html index 6d04c0634..918d44fdb 100644 --- a/templates/cfp_review/_proposals_filter_form.html +++ b/templates/cfp_review/_proposals_filter_form.html @@ -1,5 +1,6 @@ {# Filter form on /cfp-review/proposals #}
+ {% if filter_top is defined %}{{ filter_top }}{% endif %}
@@ -94,7 +95,7 @@
- Clear + Clear
diff --git a/templates/cfp_review/add_happens_relationship.html b/templates/cfp_review/add_happens_relationship.html new file mode 100644 index 000000000..e79393794 --- /dev/null +++ b/templates/cfp_review/add_happens_relationship.html @@ -0,0 +1,74 @@ +{% from "_formhelpers.html" import render_field, render_dl_field, render_radio_field %} +{% extends "cfp_review/base.html" %} +{% block title %}Add ordering relationship | {{proposal.title}}{% endblock %} +{% block body %} + +

Select a proposal which must {{ direction }} {{proposal.published_title or proposal.title}}

+ +
+
+

+ + Filter (showing {{ proposals | count }}/{{total_proposals}}) + +

+
+
+ {% set keep_qs=['happens_first_id', 'happens_later_id', 'next'] %} + {% set filter_top %} + {% for keep in keep_qs %} + {% if keep in request.args %}{% endif %} + {% endfor %} + {% endset %} + {% include "cfp_review/_proposals_filter_form.html" %} +
+
+ + + + {% if not request.args.get('reverse') %} + {% set qs_reverse_new=True %} + {% else %} + {% set qs_reverse_new=None %} + {% endif %} + + + + + + + +{% for candidate in proposals %} + + + + + + + + +{% else %} + +{% endfor %} +
+ Date + + State + + Type + + User + + Title +
{{candidate.created.strftime("%d/%m")}}{{candidate.state | capitalize}}{%- if candidate.user_scheduled %}Attendee {% endif -%}{{candidate.human_type | capitalize}}{{candidate.user.name}}{{candidate.published_title or candidate.title}} + {% set form=generate_form(candidate) %} + + {{form.hidden_tag()}} + {{form.submit(class_="btn btn-sm btn-success")}} + +
No proposals found
+ +{% endblock %} diff --git a/templates/cfp_review/update_proposal.html b/templates/cfp_review/update_proposal.html index b5d2a7075..fa8e32316 100644 --- a/templates/cfp_review/update_proposal.html +++ b/templates/cfp_review/update_proposal.html @@ -14,7 +14,8 @@

{{proposal.published_title or proposal.title}}
{% else %} A {{proposal.human_type}} by {% endif %} - {{proposal.user.name}} + {{proposal.user.name}} +

@@ -112,6 +113,48 @@

{{ render_dl_field(form.potential_venue) }} {{ render_dl_field(form.potential_time) }} {% endif %} +
+ Happens after +
+
+ This {{ proposal.human_type }} happens after: + {% if proposal.happens_after | length > 0 %} + + {% else %} + ∅ + {% endif %} + Add proposals which must happen before this one +
+
+ Happens before +
+
+ This {{ proposal.human_type }} happens before: + {% if proposal.happens_before | length > 0 %} + + {% else %} + ∅ + {% endif %} + Add proposals which must happen after this one +
@@ -209,4 +252,20 @@

Proposal actions

+{% for happens_after_proposal in proposal.happens_after %} +
+ + + +
+{% endfor %} +{% for happens_before_proposal in proposal.happens_before %} +
+ + + +
+{% endfor %} + + {% endblock %} From f19d8332c59fb03627b7dbd97a633534196f6f22 Mon Sep 17 00:00:00 2001 From: Luke Granger-Brown Date: Sun, 4 Feb 2024 19:17:11 +0000 Subject: [PATCH 4/4] Move is_safe_url/get_next_url into apps.common, and use that. --- apps/cfp_review/base.py | 3 ++- apps/common/__init__.py | 20 +++++++++++++++++++- apps/users/__init__.py | 21 +-------------------- 3 files changed, 22 insertions(+), 22 deletions(-) diff --git a/apps/cfp_review/base.py b/apps/cfp_review/base.py index 9e3c26b92..7f2e5e4b2 100644 --- a/apps/cfp_review/base.py +++ b/apps/cfp_review/base.py @@ -68,6 +68,7 @@ get_next_proposal_to, copy_request_args, ) +from ..common import get_next_url from ..common.email import from_email @@ -1251,7 +1252,7 @@ def add_happens_relationship(): flash( f"Added happens-before relationship between {happens_first.title} and {happens_later.title}.", ) - return redirect(request.args.get("next")) + return redirect(get_next_url(default=url_for(".proposals"))) if "happens_first_id" in request.args: proposal = Proposal.query.get_or_404(request.args["happens_first_id"]) diff --git a/apps/common/__init__.py b/apps/common/__init__.py index 18ff661a0..6c59a120f 100644 --- a/apps/common/__init__.py +++ b/apps/common/__init__.py @@ -5,9 +5,10 @@ import os.path from textwrap import wrap import pendulum +from urllib.parse import urlparse, urljoin from main import db, external_url -from flask import session, abort, current_app as app, render_template +from flask import session, abort, current_app as app, render_template, request, url_for from markupsafe import Markup from flask.json import jsonify from flask_login import login_user, current_user @@ -283,3 +284,20 @@ def load_archive_file(year: int, *path, raise_404=True): if json_path is None: return None return json.load(open(json_path, "r")) + + +def is_safe_url(target): + ref_url = urlparse(request.host_url) + test_url = urlparse(urljoin(request.host_url, target)) + return test_url.scheme in ("http", "https") and ref_url.netloc == test_url.netloc + + +def get_next_url(default=None): + next_url = request.args.get("next") + if next_url: + if is_safe_url(next_url): + return next_url + app.logger.error(f"Dropping unsafe next URL {repr(next_url)}") + if default is None: + default = url_for("users.account") + return default diff --git a/apps/users/__init__.py b/apps/users/__init__.py index 1da9b2ee1..219917118 100644 --- a/apps/users/__init__.py +++ b/apps/users/__init__.py @@ -1,5 +1,4 @@ import time -from urllib.parse import urlparse, urljoin from flask import ( render_template, @@ -25,7 +24,7 @@ from models.cfp import Proposal, CFPMessage from models.basket import Basket -from ..common import set_user_currency, feature_flag +from ..common import set_user_currency, feature_flag, get_next_url from ..common.email import from_email from ..common.forms import Form, EmailField @@ -57,23 +56,6 @@ def users_variables(): } -def is_safe_url(target): - ref_url = urlparse(request.host_url) - test_url = urlparse(urljoin(request.host_url, target)) - return test_url.scheme in ("http", "https") and ref_url.netloc == test_url.netloc - - -def get_next_url(default=None): - next_url = request.args.get("next") - if next_url: - if is_safe_url(next_url): - return next_url - app.logger.error(f"Dropping unsafe next URL {repr(next_url)}") - if default is None: - default = url_for(".account") - return default - - class LoginForm(Form): email = EmailField("Email") @@ -240,7 +222,6 @@ def set_currency(): @users.route("/sso/") def sso(site=None): - volunteer_sites = [app.config["VOLUNTEER_SITE"]] if "VOLUNTEER_CAMP_SITE" in app.config: volunteer_sites.append(app.config["VOLUNTEER_CAMP_SITE"])