From ec2acad472c236b20795459d9c6e17c5293e7e2b Mon Sep 17 00:00:00 2001 From: rubenwardy Date: Sat, 23 Mar 2024 16:17:26 +0000 Subject: [PATCH] Refactor package approval validation to unify implementation --- app/blueprints/packages/packages.py | 50 +---- app/logic/package_approval.py | 202 ++++++++++++++++++ app/logic/package_validator.py | 56 ----- app/models/packages.py | 64 ++---- app/templates/macros/package_approval.html | 122 +++-------- app/templates/packages/view.html | 4 +- app/tests/unit/logic/__init__.py | 0 .../test_graphs.py} | 0 app/tests/unit/logic/test_package_approval.py | 185 ++++++++++++++++ app/tests/unit/minetestcheck/__init__.py | 0 .../unit/{ => minetestcheck}/bad_args.fr.tr | 0 .../unit/{ => minetestcheck}/bad_escape.fr.tr | 0 .../{ => minetestcheck}/err_missing_eq.fr.tr | 0 .../unit/{ => minetestcheck}/foo.bar.fr.tr | 0 .../no_textdomain_comment.fr.tr | 0 .../{ => minetestcheck}/test_translation.py | 0 .../textdomain_mismatch.fr.tr | 0 .../{ => minetestcheck}/unknown_arg.fr.tr | 0 app/tests/unit/utils/__init__.py | 0 app/tests/unit/{ => utils}/test_git.py | 0 .../{ => utils}/test_minetest_hypertext.py | 0 app/tests/unit/{ => utils}/test_url.py | 0 app/tests/unit/{ => utils}/test_utils.py | 0 utils/tests.sh | 1 + 24 files changed, 443 insertions(+), 241 deletions(-) create mode 100644 app/logic/package_approval.py delete mode 100644 app/logic/package_validator.py create mode 100644 app/tests/unit/logic/__init__.py rename app/tests/unit/{test_logic_graphs.py => logic/test_graphs.py} (100%) create mode 100644 app/tests/unit/logic/test_package_approval.py create mode 100644 app/tests/unit/minetestcheck/__init__.py rename app/tests/unit/{ => minetestcheck}/bad_args.fr.tr (100%) rename app/tests/unit/{ => minetestcheck}/bad_escape.fr.tr (100%) rename app/tests/unit/{ => minetestcheck}/err_missing_eq.fr.tr (100%) rename app/tests/unit/{ => minetestcheck}/foo.bar.fr.tr (100%) rename app/tests/unit/{ => minetestcheck}/no_textdomain_comment.fr.tr (100%) rename app/tests/unit/{ => minetestcheck}/test_translation.py (100%) rename app/tests/unit/{ => minetestcheck}/textdomain_mismatch.fr.tr (100%) rename app/tests/unit/{ => minetestcheck}/unknown_arg.fr.tr (100%) create mode 100644 app/tests/unit/utils/__init__.py rename app/tests/unit/{ => utils}/test_git.py (100%) rename app/tests/unit/{ => utils}/test_minetest_hypertext.py (100%) rename app/tests/unit/{ => utils}/test_url.py (100%) rename app/tests/unit/{ => utils}/test_utils.py (100%) diff --git a/app/blueprints/packages/packages.py b/app/blueprints/packages/packages.py index 0f4da99a..53421d72 100644 --- a/app/blueprints/packages/packages.py +++ b/app/blueprints/packages/packages.py @@ -45,6 +45,7 @@ from app.models import Package, Tag, db, User, Tags, PackageState, Permission, P PackageDailyStats, Collection from app.utils import is_user_bot, get_int_or_abort, is_package_page, abs_url_for, add_audit_log, get_package_by_info, \ add_notification, get_system_user, rank_required, get_games_from_csv, get_daterange_options, post_to_approval_thread +from app.logic.package_approval import validate_package_for_approval, can_move_to_state @bp.route("/packages/") @@ -133,26 +134,6 @@ def view(package): if not package.check_perm(current_user, Permission.VIEW_PACKAGE): return render_template("packages/gone.html", package=package), 403 - show_similar = not package.approved and ( - current_user in package.maintainers or - package.check_perm(current_user, Permission.APPROVE_NEW)) - - conflicting_modnames = None - if show_similar and package.type != PackageType.TXP: - conflicting_modnames = db.session.query(MetaPackage.name) \ - .filter(MetaPackage.id.in_([ mp.id for mp in package.provides ])) \ - .filter(MetaPackage.packages.any(and_(Package.id != package.id, Package.state == PackageState.APPROVED))) \ - .all() - - conflicting_modnames += db.session.query(ForumTopic.name) \ - .filter(ForumTopic.name.in_([ mp.name for mp in package.provides ])) \ - .filter(ForumTopic.topic_id != package.forums) \ - .filter(~ db.exists().where(Package.forums==ForumTopic.topic_id)) \ - .order_by(db.asc(ForumTopic.name), db.asc(ForumTopic.title)) \ - .all() - - conflicting_modnames = set([x[0] for x in conflicting_modnames]) - packages_uses = None if package.type == PackageType.MOD: packages_uses = Package.query.filter( @@ -169,24 +150,6 @@ def view(package): if review_thread is not None and not review_thread.check_perm(current_user, Permission.SEE_THREAD): review_thread = None - topic_error = None - topic_error_lvl = "warning" - if package.state != PackageState.APPROVED and package.forums is not None: - errors = [] - if Package.query.filter(Package.forums==package.forums, Package.state!=PackageState.DELETED).count() > 1: - errors.append("" + gettext("Error: Another package already uses this forum topic!") + "") - topic_error_lvl = "danger" - - topic = ForumTopic.query.get(package.forums) - if topic is not None: - if topic.author != package.author: - errors.append("" + gettext("Error: Forum topic author doesn't match package author.") + "") - topic_error_lvl = "danger" - elif package.type != PackageType.TXP: - errors.append(gettext("Warning: Forum topic not found. This may happen if the topic has only just been created.")) - - topic_error = "
".join(errors) - threads = Thread.query.filter_by(package_id=package.id, review_id=None) if not current_user.is_authenticated: threads = threads.filter_by(private=False) @@ -196,6 +159,10 @@ def view(package): has_review = current_user.is_authenticated and \ PackageReview.query.filter_by(package=package, author=current_user).count() > 0 + validation = None + if package.state != PackageState.APPROVED: + validation = validate_package_for_approval(package) + is_favorited = current_user.is_authenticated and \ Collection.query.filter( Collection.author == current_user, @@ -204,9 +171,8 @@ def view(package): return render_template("packages/view.html", package=package, releases=releases, packages_uses=packages_uses, - conflicting_modnames=conflicting_modnames, - review_thread=review_thread, topic_error=topic_error, topic_error_lvl=topic_error_lvl, - threads=threads.all(), has_review=has_review, is_favorited=is_favorited) + review_thread=review_thread, threads=threads.all(), validation=validation, + has_review=has_review, is_favorited=is_favorited) @bp.route("/packages///shields//") @@ -421,7 +387,7 @@ def move_to_state(package): if state is None: abort(400) - if not package.can_move_to_state(current_user, state): + if not can_move_to_state(package, current_user, state): flash(gettext("You don't have permission to do that"), "danger") return redirect(package.get_url("packages.view")) diff --git a/app/logic/package_approval.py b/app/logic/package_approval.py new file mode 100644 index 00000000..b9fa4b35 --- /dev/null +++ b/app/logic/package_approval.py @@ -0,0 +1,202 @@ +# ContentDB +# Copyright (C) rubenwardy +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. +# +# You should have received a copy of the GNU Affero General Public License +# along with this program. If not, see . + +from typing import List, Tuple, Union, Optional + +from flask_babel import lazy_gettext, LazyString +from sqlalchemy import and_, or_ + +from app.models import Package, PackageType, PackageState, PackageRelease, db, MetaPackage, ForumTopic, User, \ + Permission, UserRank + + +class PackageValidationNote: + # level is danger, warning, or info + level: str + message: LazyString + buttons: List[Tuple[str, LazyString]] + + # False to prevent "Approve" + allow_approval: bool + + # False to prevent "Submit for Approval" + allow_submit: bool + + def __init__(self, level: str, message: LazyString, allow_approval: bool, allow_submit: bool): + self.level = level + self.message = message + self.buttons = [] + self.allow_approval = allow_approval + self.allow_submit = allow_submit + + def add_button(self, url: str, label: LazyString) -> "PackageValidationNote": + self.buttons.append((url, label)) + return self + + +def is_package_name_taken(normalised_name: str) -> bool: + return Package.query.filter( + and_(Package.state == PackageState.APPROVED, + or_(Package.name == normalised_name, + Package.name == normalised_name + "_game"))).count() > 0 + + +def get_conflicting_mod_names(package: Package) -> set[str]: + conflicting_modnames = (db.session.query(MetaPackage.name) + .filter(MetaPackage.id.in_([mp.id for mp in package.provides])) + .filter(MetaPackage.packages.any(and_(Package.id != package.id, Package.state == PackageState.APPROVED))) + .all()) + conflicting_modnames += (db.session.query(ForumTopic.name) + .filter(ForumTopic.name.in_([mp.name for mp in package.provides])) + .filter(ForumTopic.topic_id != package.forums) + .filter(~ db.exists().where(Package.forums == ForumTopic.topic_id)) + .order_by(db.asc(ForumTopic.name), db.asc(ForumTopic.title)) + .all()) + return set([x[0] for x in conflicting_modnames]) + + +def count_packages_with_forum_topic(topic_id: int) -> int: + return Package.query.filter(Package.forums == topic_id, Package.state != PackageState.DELETED).count() > 1 + + +def get_forum_topic(topic_id: int) -> Optional[ForumTopic]: + return ForumTopic.query.get(topic_id) + + +def validate_package_for_approval(package: Package) -> List[PackageValidationNote]: + retval: List[PackageValidationNote] = [] + + def template(level: str, allow_approval: bool, allow_submit: bool): + def inner(msg: LazyString): + note = PackageValidationNote(level, msg, allow_approval, allow_submit) + retval.append(note) + return note + + return inner + + danger = template("danger", allow_approval=False, allow_submit=False) + warning = template("warning", allow_approval=True, allow_submit=True) + info = template("info", allow_approval=False, allow_submit=True) + + if package.type != PackageType.MOD and is_package_name_taken(package.normalised_name): + danger(lazy_gettext("A package already exists with this name. Please see Policy and Guidance 3")) + + if package.releases.filter(PackageRelease.task_id.is_(None)).count() == 0: + if package.releases.count() == 0: + message = lazy_gettext("You need to create a release before this package can be approved.") + else: + message = lazy_gettext("Release is still importing, or has an error.") + + danger(message) \ + .add_button(package.get_url("packages.create_release"), lazy_gettext("Create release")) \ + .add_button(package.get_url("packages.setup_releases"), lazy_gettext("Set up releases")) + + # Don't bother validating any more until we have a release + return retval + + if (package.type == PackageType.GAME or package.type == PackageType.TXP) and \ + package.screenshots.count() == 0: + danger(lazy_gettext("You need to add at least one screenshot.")) + + missing_deps = package.get_missing_hard_dependencies_query().all() + if len(missing_deps) > 0: + missing_deps = ", ".join([ x.name for x in missing_deps]) + danger(lazy_gettext( + "The following hard dependencies need to be added to ContentDB first: %(deps)s", deps=missing_deps)) + + if package.type != PackageType.GAME and not package.supports_all_games and package.supported_games.count() == 0: + danger(lazy_gettext( + "What games does your package support? Please specify on the supported games page", deps=missing_deps)) \ + .add_button(package.get_url("packages.game_support"), lazy_gettext("Supported Games")) + + if "Other" in package.license.name or "Other" in package.media_license.name: + info(lazy_gettext("Please wait for the license to be added to CDB.")) + + # Check similar mod name + conflicting_modnames = set() + if package.type != PackageType.TXP: + conflicting_modnames = get_conflicting_mod_names(package) + + if len(conflicting_modnames) > 4: + warning(lazy_gettext("Please make sure that this package has the right to the names it uses.")) + elif len(conflicting_modnames) > 0: + names_list = list(conflicting_modnames) + names_list.sort() + warning(lazy_gettext("Please make sure that this package has the right to the names %(names)s", + names=", ".join(names_list))) \ + .add_button(package.get_url('packages.similar'), lazy_gettext("See more")) + + # Check forum topic + if package.state != PackageState.APPROVED and package.forums is not None: + if count_packages_with_forum_topic(package.forums) > 1: + danger("" + lazy_gettext("Error: Another package already uses this forum topic!") + "") + + topic = get_forum_topic(package.forums) + if topic is not None: + if topic.author != package.author: + danger("" + lazy_gettext("Error: Forum topic author doesn't match package author.") + "") + elif package.type != PackageType.TXP: + warning(lazy_gettext("Warning: Forum topic not found. The topic may have been created since the last forum crawl.")) + + return retval + + +PACKAGE_STATE_FLOW = { + PackageState.WIP: {PackageState.READY_FOR_REVIEW}, + PackageState.CHANGES_NEEDED: {PackageState.READY_FOR_REVIEW}, + PackageState.READY_FOR_REVIEW: {PackageState.WIP, PackageState.CHANGES_NEEDED, PackageState.APPROVED}, + PackageState.APPROVED: {PackageState.CHANGES_NEEDED}, + PackageState.DELETED: {PackageState.READY_FOR_REVIEW}, +} + + +def can_move_to_state(package: Package, user: User, new_state: Union[str, PackageState]) -> bool: + if not user.is_authenticated: + return False + + if type(new_state) == str: + new_state = PackageState[new_state] + elif type(new_state) != PackageState: + raise Exception("Unknown state given to can_move_to_state()") + + if new_state not in PACKAGE_STATE_FLOW[package.state]: + return False + + if new_state == PackageState.READY_FOR_REVIEW or new_state == PackageState.APPROVED: + # Can the user approve? + if new_state == PackageState.APPROVED and not package.check_perm(user, Permission.APPROVE_NEW): + return False + + # Must be able to edit or approve package to change its state + if not (package.check_perm(user, Permission.APPROVE_NEW) or package.check_perm(user, Permission.EDIT_PACKAGE)): + return False + + # Are there any validation warnings? + validation_notes = validate_package_for_approval(package) + for note in validation_notes: + if not note.allow_submit or (new_state == PackageState.APPROVED and not note.allow_approval): + return False + + return True + + elif new_state == PackageState.CHANGES_NEEDED: + return package.check_perm(user, Permission.APPROVE_NEW) + + elif new_state == PackageState.WIP: + return package.check_perm(user, Permission.EDIT_PACKAGE) and \ + (user in package.maintainers or user.rank.at_least(UserRank.ADMIN)) + + return True diff --git a/app/logic/package_validator.py b/app/logic/package_validator.py deleted file mode 100644 index db68f880..00000000 --- a/app/logic/package_validator.py +++ /dev/null @@ -1,56 +0,0 @@ -# ContentDB -# Copyright (C) rubenwardy -# -# This program is free software: you can redistribute it and/or modify -# it under the terms of the GNU Affero General Public License as published by -# the Free Software Foundation, either version 3 of the License, or -# (at your option) any later version. -# -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU Affero General Public License for more details. -# -# You should have received a copy of the GNU Affero General Public License -# along with this program. If not, see . - -from collections import namedtuple -from typing import List - -from flask_babel import lazy_gettext -from sqlalchemy import and_, or_ - -from app.models import Package, PackageType, PackageState, PackageRelease - - -ValidationError = namedtuple("ValidationError", "status message") - - -def validate_package_for_approval(package: Package) -> List[ValidationError]: - retval: List[ValidationError] = [] - - normalised_name = package.getNormalisedName() - if package.type != PackageType.MOD and Package.query.filter( - and_(Package.state == PackageState.APPROVED, - or_(Package.name == normalised_name, - Package.name == normalised_name + "_game"))).count() > 0: - retval.append(("danger", lazy_gettext("A package already exists with this name. Please see Policy and Guidance 3"))) - - if package.releases.filter(PackageRelease.task_id == None).count() == 0: - retval.append(("danger", lazy_gettext("A release is required before this package can be approved."))) - # Don't bother validating any more until we have a release - return retval - - missing_deps = package.get_missing_hard_dependencies_query().all() - if len(missing_deps) > 0: - retval.append(("danger", lazy_gettext( - "The following hard dependencies need to be added to ContentDB first: %(deps)s", deps=missing_deps))) - - if (package.type == package.type.GAME or package.type == package.type.TXP) and \ - package.screenshots.count() == 0: - retval.append(("danger", lazy_gettext("You need to add at least one screenshot."))) - - if "Other" in package.license.name or "Other" in package.media_license.name: - retval.append(("info", lazy_gettext("Please wait for the license to be added to CDB."))) - - return retval diff --git a/app/models/packages.py b/app/models/packages.py index fc0bef62..13e15c91 100644 --- a/app/models/packages.py +++ b/app/models/packages.py @@ -208,15 +208,6 @@ class PackageState(enum.Enum): return item if type(item) == PackageState else PackageState[item.upper()] -PACKAGE_STATE_FLOW = { - PackageState.WIP: {PackageState.READY_FOR_REVIEW}, - PackageState.CHANGES_NEEDED: {PackageState.READY_FOR_REVIEW}, - PackageState.READY_FOR_REVIEW: {PackageState.WIP, PackageState.CHANGES_NEEDED, PackageState.APPROVED}, - PackageState.APPROVED: {PackageState.CHANGES_NEEDED}, - PackageState.DELETED: {PackageState.READY_FOR_REVIEW}, -} - - PackageProvides = db.Table("provides", db.Column("package_id", db.Integer, db.ForeignKey("package.id"), primary_key=True), db.Column("metapackage_id", db.Integer, db.ForeignKey("meta_package.id"), primary_key=True) @@ -487,13 +478,19 @@ class Package(db.Model): if name.endswith("_game"): name = name[:-5] - return Package.query.filter( - or_(Package.name == name, Package.name == name + "_game"), + return Package.query.filter(or_(Package.name == name, Package.name == name + "_game"), Package.author.has(username=parts[0])).first() def get_id(self): return "{}/{}".format(self.author.username, self.name) + @property + def normalised_name(self): + name = self.name + if name.endswith("_game"): + name = name[:-5] + return name + def get_translated(self, lang=None, load_desc=True): if lang is None: locale = get_locale() @@ -675,7 +672,7 @@ class Package(db.Model): if type(state) == str: state = PackageState[state] elif type(state) != PackageState: - raise Exception("Unknown state given to Package.can_move_to_state()") + raise Exception("Unknown state given to Package.get_set_state_url()") return url_for("packages.move_to_state", author=self.author.username, name=self.name, state=state.name.lower()) @@ -751,50 +748,13 @@ class Package(db.Model): def get_missing_hard_dependencies(self): return [mp.name for mp in self.get_missing_hard_dependencies_query().all()] - def can_move_to_state(self, user, state): - if not user.is_authenticated: - return False - - if type(state) == str: - state = PackageState[state] - elif type(state) != PackageState: - raise Exception("Unknown state given to Package.can_move_to_state()") - - if state not in PACKAGE_STATE_FLOW[self.state]: - return False - - if state == PackageState.READY_FOR_REVIEW or state == PackageState.APPROVED: - if state == PackageState.APPROVED and not self.check_perm(user, Permission.APPROVE_NEW): - return False - - if not (self.check_perm(user, Permission.APPROVE_NEW) or self.check_perm(user, Permission.EDIT_PACKAGE)): - return False - - if state == PackageState.APPROVED and ("Other" in self.license.name or "Other" in self.media_license.name): - return False - - if self.get_missing_hard_dependencies_query().count() > 0: - return False - - needs_screenshot = \ - (self.type == self.type.GAME or self.type == self.type.TXP) and self.screenshots.count() == 0 - - return self.releases.filter(PackageRelease.task_id==None).count() > 0 and not needs_screenshot - - elif state == PackageState.CHANGES_NEEDED: - return self.check_perm(user, Permission.APPROVE_NEW) - - elif state == PackageState.WIP: - return self.check_perm(user, Permission.EDIT_PACKAGE) and \ - (user in self.maintainers or user.rank.at_least(UserRank.ADMIN)) - - return True - def get_next_states(self, user): + from app.logic.package_approval import can_move_to_state + states = [] for state in PackageState: - if self.can_move_to_state(user, state): + if can_move_to_state(self, user, state): states.append(state) return states diff --git a/app/templates/macros/package_approval.html b/app/templates/macros/package_approval.html index 48e3fb37..c3ce7a54 100644 --- a/app/templates/macros/package_approval.html +++ b/app/templates/macros/package_approval.html @@ -1,4 +1,4 @@ -{% macro render_banners(package, current_user, topic_error, topic_error_lvl, conflicting_modnames) -%} +{% macro render_banners(package, current_user, validation) -%}
@@ -13,98 +13,42 @@ {% endfor %}
-{% set level = "warning" %} -{% if package.releases.filter_by(task_id=None).count() == 0 %} - {% set message %} - {% if package.check_perm(current_user, "MAKE_RELEASE") %} - {% if package.update_config %} - - {{ _("Create release") }} - - {% else %} - - {{ _("Set up releases") }} - - {% endif %} - - {% if package.releases.count() == 0 %} - {{ _("You need to create a release before this package can be approved.") }} - {% else %} - {{ _("Release is still importing, or has an error.") }} - {% endif %} - {% else %} - {{ _("A release is required before this package can be approved.") }} - {% endif %} - {% endset %} - -{% elif (package.type == package.type.GAME or package.type == package.type.TXP) and package.screenshots.count() == 0 %} - {% set message = _("You need to add at least one screenshot.") %} - -{% elif package.get_missing_hard_dependencies_query().count() > 0 %} - {% set deps = package.get_missing_hard_dependencies() | join(", ") %} - {% set message = _("The following hard dependencies need to be added to ContentDB first: %(deps)s", deps=deps) %} - -{% elif topic_error_lvl == "danger" %} -{% elif package.state == package.state.READY_FOR_REVIEW and ("Other" in package.license.name or "Other" in package.media_license.name) %} - {% set message = _("Please wait for the license to be added to CDB.") %} - -{% else %} - {% set level = "info" %} - {% set message %} - {% if package.screenshots.count() == 0 %} - - {{ _("You should add at least one screenshot.") }} -
- {% endif %} - - {% if package.state == package.state.READY_FOR_REVIEW %} - {% if not package.get_download_release() %} - {{ _("Please wait for the release to be approved.") }} - {% elif package.check_perm(current_user, "APPROVE_NEW") %} - {{ _("You can now approve this package if you're ready.") }} - {% else %} - {{ _("Please wait for the package to be approved.") }} - {% endif %} - {% else %} - {% if package.check_perm(current_user, "EDIT_PACKAGE") %} - {{ _("You can now submit this package for approval if you're ready.") }} - {% else %} - {{ _("This package can be submitted for approval when ready.") }} - {% endif %} - {% endif %} - {% endset %} -{% endif %} - -{% if message %} -
- - - {{ message | safe }} - -
+{% for note in validation %} +
+
+
+ {{ note.message }} +
+
+ {% for button in note.buttons %} + + {{ button[1] }} + + {% endfor %} +
+
-{% endif %} +{% endfor %} -{% if topic_error %} -
- - {{ topic_error | safe }} -
-
-{% endif %} - -{% if conflicting_modnames %} -
- - More info - - {% if conflicting_modnames | length > 4 %} - {{ _("Please make sure that this package has the right to the names it uses.") }} +

+ {% if package.state == package.state.READY_FOR_REVIEW %} + {% if not package.get_download_release() %} + {{ _("Please wait for the release to be approved.") }} + {% elif package.check_perm(current_user, "APPROVE_NEW") %} + {{ _("You can now approve this package if you're ready.") }} {% else %} - {{ _("Please make sure that this package has the right to the names %(names)s", names=conflicting_modnames | join(", ")) }}. + {{ _("Please wait for the package to be approved.") }} {% endif %} -

-{% endif %} + {% elif package.state.READY_FOR_REVIEW in package.get_next_states(current_user) %} + {% if package.check_perm(current_user, "EDIT_PACKAGE") %} + {{ _("You can now submit this package for approval if you're ready.") }} + {% else %} + {{ _("This package can be submitted for approval when ready.") }} + {% endif %} + {% else %} + {{ _("You need to fix the above errors before you can submit for review") }} + {% endif %} +

{% if not package.review_thread and (package.author == current_user or package.check_perm(current_user, "APPROVE_NEW")) %}
diff --git a/app/templates/packages/view.html b/app/templates/packages/view.html index 7910c97c..5b634c1f 100644 --- a/app/templates/packages/view.html +++ b/app/templates/packages/view.html @@ -85,7 +85,7 @@
{% from "macros/package_approval.html" import render_banners %} - {{ render_banners(package, current_user, topic_error, topic_error_lvl, conflicting_modnames) }} + {{ render_banners(package, current_user, validation) }} {% if review_thread and review_thread.check_perm(current_user, "SEE_THREAD") %}

{% if review_thread.private %}🔒{% endif %} {{ review_thread.title }}

@@ -532,7 +532,7 @@

{{ _("No specific game required") }}

- {% if package.check_perm(current_user, "EDIT_PACKAGE") %} + {% if package.state == package.state.APPROVED and package.check_perm(current_user, "EDIT_PACKAGE") %}

{{ _("Is the above correct?") }} diff --git a/app/tests/unit/logic/__init__.py b/app/tests/unit/logic/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/app/tests/unit/test_logic_graphs.py b/app/tests/unit/logic/test_graphs.py similarity index 100% rename from app/tests/unit/test_logic_graphs.py rename to app/tests/unit/logic/test_graphs.py diff --git a/app/tests/unit/logic/test_package_approval.py b/app/tests/unit/logic/test_package_approval.py new file mode 100644 index 00000000..dbff535b --- /dev/null +++ b/app/tests/unit/logic/test_package_approval.py @@ -0,0 +1,185 @@ +# ContentDB +# Copyright (C) rubenwardy +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. +# +# You should have received a copy of the GNU Affero General Public License +# along with this program. If not, see . + +from unittest.mock import MagicMock, patch + +from app.logic import package_approval +from app.models import Package, PackageType + + +class MockPackageHelper: + package: Package + + def __init__(self, type_: PackageType = PackageType.MOD): + self.package = MagicMock() + + self.package.type = type_ + self.package.name = "foobar" + self.package.normalised_name.name = "foobar" + self.package.author.username = "username" + self.package.author.id = 3 + + self.package.releases.filter.return_value.count.return_value = 0 + self.package.releases.count.return_value = 0 + self.package.get_url.return_value = "hi" + self.package.screenshots.count.return_value = 0 + + def add_release(self): + self.package.releases.filter.return_value.count.return_value = 1 + self.package.releases.count.return_value = 1 + + def add_pending_release(self): + self.package.releases.filter.return_value.count.return_value = 0 + self.package.releases.count.return_value = 1 + + def add_screenshot(self): + self.package.screenshots.count.return_value = 1 + + def add_missing_hard_deps(self): + mod_name = MagicMock() + mod_name.name = "missing" + self.package.get_missing_hard_dependencies_query.return_value.all.return_value = [mod_name] + + def set_license(self, code_license: str, media_license: str): + self.package.license.name = code_license + self.package.media_license.name = media_license + + def set_no_game_support(self): + assert self.package.type != PackageType.GAME + self.package.supports_all_games = False + self.package.supported_games.count.return_value = 0 + + +def test_requires_release(): + mock_package = MockPackageHelper() + + notes = package_approval.validate_package_for_approval(mock_package.package) + assert len(notes) == 1 + assert notes[0].message == "You need to create a release before this package can be approved." + + mock_package.add_pending_release() + notes = package_approval.validate_package_for_approval(mock_package.package) + assert len(notes) == 1 + assert notes[0].message == "Release is still importing, or has an error." + + +@patch("app.logic.package_approval.is_package_name_taken", MagicMock(return_value=False)) +@patch("app.logic.package_approval.get_conflicting_mod_names", MagicMock(return_value=set())) +@patch("app.logic.package_approval.count_packages_with_forum_topic", MagicMock(return_value=1)) +@patch("app.logic.package_approval.get_forum_topic") +def test_missing_hard_deps(get_forum_topic): + mock_package = MockPackageHelper(PackageType.MOD) + mock_package.add_release() + mock_package.add_missing_hard_deps() + + topic = MagicMock() + topic.author = mock_package.package.author + get_forum_topic.return_value = topic + + notes = package_approval.validate_package_for_approval(mock_package.package) + assert len(notes) == 1 + assert notes[0].message == "The following hard dependencies need to be added to ContentDB first: missing" + + +@patch("app.logic.package_approval.is_package_name_taken", MagicMock(return_value=True)) +@patch("app.logic.package_approval.get_conflicting_mod_names", MagicMock(return_value={"one", "two"})) +@patch("app.logic.package_approval.count_packages_with_forum_topic", MagicMock(return_value=2)) +@patch("app.logic.package_approval.get_forum_topic", MagicMock(return_value=None)) +def test_requires_multiple_issues(): + mock_package = MockPackageHelper() + mock_package.add_release() + mock_package.set_license("Other", "Other") + mock_package.set_no_game_support() + + notes = package_approval.validate_package_for_approval(mock_package.package) + assert len(notes) == 5 + assert notes[0].message == "What games does your package support? Please specify on the supported games page" + assert notes[1].message == "Please wait for the license to be added to CDB." + assert notes[2].message == "Please make sure that this package has the right to the names one, two" + assert notes[3].message == "Error: Another package already uses this forum topic!" + assert notes[4].message == "Warning: Forum topic not found. The topic may have been created since the last forum crawl." + + +@patch("app.logic.package_approval.is_package_name_taken", MagicMock(return_value=False)) +@patch("app.logic.package_approval.get_conflicting_mod_names", MagicMock(return_value=set())) +@patch("app.logic.package_approval.count_packages_with_forum_topic", MagicMock(return_value=1)) +@patch("app.logic.package_approval.get_forum_topic") +def test_forum_topic_author_mismatch(get_forum_topic): + mock_package = MockPackageHelper() + mock_package.add_release() + + topic = MagicMock() + get_forum_topic.return_value = topic + + notes = package_approval.validate_package_for_approval(mock_package.package) + assert len(notes) == 1 + assert notes[0].message == "Error: Forum topic author doesn't match package author." + + +@patch("app.logic.package_approval.is_package_name_taken", MagicMock(return_value=False)) +@patch("app.logic.package_approval.get_conflicting_mod_names", MagicMock(return_value=set())) +@patch("app.logic.package_approval.count_packages_with_forum_topic", MagicMock(return_value=1)) +@patch("app.logic.package_approval.get_forum_topic") +def test_passes(get_forum_topic): + mock_package = MockPackageHelper() + mock_package.add_release() + + topic = MagicMock() + topic.author = mock_package.package.author + get_forum_topic.return_value = topic + + notes = package_approval.validate_package_for_approval(mock_package.package) + assert len(notes) == 0 + + +@patch("app.logic.package_approval.is_package_name_taken", MagicMock(return_value=True)) +@patch("app.logic.package_approval.get_conflicting_mod_names", MagicMock(return_value=set())) +@patch("app.logic.package_approval.count_packages_with_forum_topic", MagicMock(return_value=1)) +@patch("app.logic.package_approval.get_forum_topic") +def test_games_txp_must_have_unique_name(get_forum_topic): + mock_package = MockPackageHelper(PackageType.GAME) + mock_package.add_release() + mock_package.add_screenshot() + + topic = MagicMock() + topic.author = mock_package.package.author + get_forum_topic.return_value = topic + + notes = package_approval.validate_package_for_approval(mock_package.package) + + assert len(notes) == 1 + assert notes[0].message == "A package already exists with this name. Please see Policy and Guidance 3" + + +@patch("app.logic.package_approval.is_package_name_taken", MagicMock(return_value=False)) +@patch("app.logic.package_approval.get_conflicting_mod_names", MagicMock(return_value=set())) +@patch("app.logic.package_approval.count_packages_with_forum_topic", MagicMock(return_value=1)) +@patch("app.logic.package_approval.get_forum_topic") +def test_games_txp_require_screenshots(get_forum_topic): + mock_package = MockPackageHelper(PackageType.GAME) + mock_package.add_release() + + topic = MagicMock() + topic.author = mock_package.package.author + get_forum_topic.return_value = topic + + notes = package_approval.validate_package_for_approval(mock_package.package) + assert len(notes) == 1 + assert notes[0].message == "You need to add at least one screenshot." + + mock_package.add_screenshot() + notes = package_approval.validate_package_for_approval(mock_package.package) + assert len(notes) == 0 diff --git a/app/tests/unit/minetestcheck/__init__.py b/app/tests/unit/minetestcheck/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/app/tests/unit/bad_args.fr.tr b/app/tests/unit/minetestcheck/bad_args.fr.tr similarity index 100% rename from app/tests/unit/bad_args.fr.tr rename to app/tests/unit/minetestcheck/bad_args.fr.tr diff --git a/app/tests/unit/bad_escape.fr.tr b/app/tests/unit/minetestcheck/bad_escape.fr.tr similarity index 100% rename from app/tests/unit/bad_escape.fr.tr rename to app/tests/unit/minetestcheck/bad_escape.fr.tr diff --git a/app/tests/unit/err_missing_eq.fr.tr b/app/tests/unit/minetestcheck/err_missing_eq.fr.tr similarity index 100% rename from app/tests/unit/err_missing_eq.fr.tr rename to app/tests/unit/minetestcheck/err_missing_eq.fr.tr diff --git a/app/tests/unit/foo.bar.fr.tr b/app/tests/unit/minetestcheck/foo.bar.fr.tr similarity index 100% rename from app/tests/unit/foo.bar.fr.tr rename to app/tests/unit/minetestcheck/foo.bar.fr.tr diff --git a/app/tests/unit/no_textdomain_comment.fr.tr b/app/tests/unit/minetestcheck/no_textdomain_comment.fr.tr similarity index 100% rename from app/tests/unit/no_textdomain_comment.fr.tr rename to app/tests/unit/minetestcheck/no_textdomain_comment.fr.tr diff --git a/app/tests/unit/test_translation.py b/app/tests/unit/minetestcheck/test_translation.py similarity index 100% rename from app/tests/unit/test_translation.py rename to app/tests/unit/minetestcheck/test_translation.py diff --git a/app/tests/unit/textdomain_mismatch.fr.tr b/app/tests/unit/minetestcheck/textdomain_mismatch.fr.tr similarity index 100% rename from app/tests/unit/textdomain_mismatch.fr.tr rename to app/tests/unit/minetestcheck/textdomain_mismatch.fr.tr diff --git a/app/tests/unit/unknown_arg.fr.tr b/app/tests/unit/minetestcheck/unknown_arg.fr.tr similarity index 100% rename from app/tests/unit/unknown_arg.fr.tr rename to app/tests/unit/minetestcheck/unknown_arg.fr.tr diff --git a/app/tests/unit/utils/__init__.py b/app/tests/unit/utils/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/app/tests/unit/test_git.py b/app/tests/unit/utils/test_git.py similarity index 100% rename from app/tests/unit/test_git.py rename to app/tests/unit/utils/test_git.py diff --git a/app/tests/unit/test_minetest_hypertext.py b/app/tests/unit/utils/test_minetest_hypertext.py similarity index 100% rename from app/tests/unit/test_minetest_hypertext.py rename to app/tests/unit/utils/test_minetest_hypertext.py diff --git a/app/tests/unit/test_url.py b/app/tests/unit/utils/test_url.py similarity index 100% rename from app/tests/unit/test_url.py rename to app/tests/unit/utils/test_url.py diff --git a/app/tests/unit/test_utils.py b/app/tests/unit/utils/test_utils.py similarity index 100% rename from app/tests/unit/test_utils.py rename to app/tests/unit/utils/test_utils.py diff --git a/utils/tests.sh b/utils/tests.sh index 421f5a50..f6b5fc04 100755 --- a/utils/tests.sh +++ b/utils/tests.sh @@ -3,4 +3,5 @@ set -e . "${BASH_SOURCE%/*}/common.sh" +# To do a specific test file, change the path docker exec "$(container app)" sh -c "FLASK_CONFIG=../config.cfg FLASK_APP=app/__init__.py python -m pytest app/tests/ --disable-warnings"