Refactor package approval validation to unify implementation

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("<b>" + gettext("Error: Another package already uses this forum topic!") + "</b>")
-			topic_error_lvl = "danger"
-
-		topic = ForumTopic.query.get(package.forums)
-		if topic is not None:
-			if topic.author != package.author:
-				errors.append("<b>" + gettext("Error: Forum topic author doesn't match package author.") + "</b>")
-				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 = "<br />".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/<author>/<name>/shields/<type>/")
@@ -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"))
 

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 <https://www.gnu.org/licenses/>.
+
+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("<b>" + lazy_gettext("Error: Another package already uses this forum topic!") + "</b>")
+
+		topic = get_forum_topic(package.forums)
+		if topic is not None:
+			if topic.author != package.author:
+				danger("<b>" + lazy_gettext("Error: Forum topic author doesn't match package author.") + "</b>")
+		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

app/logic/package_validator.py

@@ -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 <https://www.gnu.org/licenses/>.
-
-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

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

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) -%}
 
 <div class="row mb-4">
 	<span class="col">
@@ -13,98 +13,42 @@
 	{% endfor %}
 </div>
 
-{% 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 %}
-				<a class="btn btn-sm btn-warning float-end" href="{{ package.get_url('packages.create_release') }}">
-					{{ _("Create release") }}
-				</a>
-			{% else %}
-				<a class="btn btn-sm btn-warning float-end" href="{{ package.get_url('packages.setup_releases') }}">
-					{{ _("Set up releases") }}
-				</a>
-			{% 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 %}
-			<b>
-				{{ _("You should add at least one screenshot.") }}
-			</b><br />
-		{% 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 %}
-	<div class="alert alert-{{ level }}">
-		<span class="icon_message"></span>
-
-		{{ message | safe }}
-
-		<div style="clear: both;"></div>
+{% for note in validation %}
+	<div class="alert alert-{{ note.level }}">
+		<div class="row g-3">
+			<div class="col-md">
+				{{ note.message }}
+			</div>
+			<div class="col-md-auto">
+				{% for button in note.buttons %}
+					<a href="{{ button[0] }}" class="btn btn-sm btn-{{ note.level }} ms-2">
+						{{ button[1] }}
+					</a>
+				{% endfor %}
+			</div>
+		</div>
 	</div>
-{% endif %}
+{% endfor %}
 
-{% if topic_error %}
-	<div class="alert alert-{{ topic_error_lvl }}">
-		<span class="icon_message"></span>
-		{{ topic_error | safe }}
-		<div style="clear: both;"></div>
-	</div>
-{% endif %}
-
-{% if conflicting_modnames %}
-	<div class="alert alert-warning">
-		<a class="float-end btn btn-sm btn-warning" href="{{ package.get_url('packages.similar') }}">
-			More info
-		</a>
-		{% if conflicting_modnames | length > 4 %}
-			{{ _("Please make sure that this package has the right to the names it uses.") }}
+<p class="alert alert-secondary">
+	{% 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 %}
-	</div>
-{% 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 %}
+</p>
 
 {% if not package.review_thread and (package.author == current_user or package.check_perm(current_user, "APPROVE_NEW")) %}
 	<div class="alert alert-secondary">

app/templates/packages/view.html

@@ -85,7 +85,7 @@
 		<section class="my-4 pb-3" style="">
 			<div class="container">
 				{% 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") %}
 					<h2>{% if review_thread.private %}&#x1f512;{% endif %} {{ review_thread.title }}</h2>
@@ -532,7 +532,7 @@
 							<p>
 								{{ _("No specific game required") }}
 							</p>
-							{% if package.check_perm(current_user, "EDIT_PACKAGE") %}
+							{% if package.state == package.state.APPROVED and package.check_perm(current_user, "EDIT_PACKAGE") %}
 								<div class="alert alert-warning">
 									<p>
 										{{ _("Is the above correct?") }}

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 <https://www.gnu.org/licenses/>.
+
+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 == "<b>Error: Another package already uses this forum topic!</b>"
+	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 == "<b>Error: Forum topic author doesn't match package author.</b>"
+
+
+@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

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"