diff --git a/tests/unit/admin/views/test_classifiers.py b/tests/unit/admin/views/test_classifiers.py index c705251ddc01..81d18bbaaedc 100644 --- a/tests/unit/admin/views/test_classifiers.py +++ b/tests/unit/admin/views/test_classifiers.py @@ -13,6 +13,8 @@ import pretend import pytest +from webob.multidict import MultiDict + from warehouse.admin.views import classifiers as views from warehouse.classifiers.models import Classifier @@ -82,10 +84,28 @@ def test_add_parent_classifier(self, db_request): class TestDeprecateClassifier: - def test_deprecate_classifier(self, db_request): - classifier = ClassifierFactory(deprecated=False) + @pytest.mark.parametrize("has_alternative_classifiers", [(True,), (False,)]) + def test_deprecate_classifier(self, db_request, has_alternative_classifiers): + classifier = ClassifierFactory( + classifier="Classifier :: For Testing", deprecated=False + ) + + db_request.params = MultiDict({"classifier_id": classifier.id}) + + if has_alternative_classifiers: + classifier.alternatives.extend( + [ + ClassifierFactory(classifier="AA :: Alternative", deprecated=True), + ClassifierFactory(classifier="BB :: Alternative", deprecated=False), + ] + ) + db_request.params.extend( + [ + ("deprecated_by", alternative.id) + for alternative in classifier.alternatives + ] + ) - db_request.params = {"classifier_id": classifier.id} db_request.session.flash = pretend.call_recorder(lambda *a, **kw: None) db_request.route_path = lambda *a: "/the/path" @@ -93,3 +113,8 @@ def test_deprecate_classifier(self, db_request): db_request.db.flush() assert classifier.deprecated + assert db_request.session.flash.calls == [ + pretend.call( + "Deprecated classifier 'Classifier :: For Testing'", queue="success" + ) + ] diff --git a/tests/unit/forklift/test_legacy.py b/tests/unit/forklift/test_legacy.py index cc6e6eefa542..d46861642d22 100644 --- a/tests/unit/forklift/test_legacy.py +++ b/tests/unit/forklift/test_legacy.py @@ -1570,7 +1570,31 @@ def test_upload_fails_with_invalid_classifier(self, pyramid_config, db_request): "for this field" ) - def test_upload_fails_with_deprecated_classifier(self, pyramid_config, db_request): + @pytest.mark.parametrize( + "has_alternative_classifiers, expected_message", + [ + ( + False, + ( + "400 Invalid value for classifiers. " + "Error: Classifier 'AA :: BB' has been deprecated, see /url " + "for a list of valid classifiers." + ), + ), + ( + True, + ( + "400 Invalid value for classifiers. " + "Error: Classifier 'AA :: BB' has been deprecated " + "in favor of the following classifier(s): " + "'AA :: Alternative', 'BB :: Alternative'" + ), + ), + ], + ) + def test_upload_fails_with_deprecated_classifier( + self, pyramid_config, db_request, has_alternative_classifiers, expected_message + ): pyramid_config.testing_securitypolicy(userid=1) user = UserFactory.create() @@ -1581,6 +1605,14 @@ def test_upload_fails_with_deprecated_classifier(self, pyramid_config, db_reques RoleFactory.create(user=user, project=project) classifier = ClassifierFactory(classifier="AA :: BB", deprecated=True) + if has_alternative_classifiers: + classifier.alternatives.extend( + [ + ClassifierFactory(classifier="AA :: Alternative", deprecated=True), + ClassifierFactory(classifier="BB :: Alternative", deprecated=False), + ] + ) + filename = "{}-{}.tar.gz".format(project.name, release.version) db_request.POST = MultiDict( @@ -1606,11 +1638,7 @@ def test_upload_fails_with_deprecated_classifier(self, pyramid_config, db_reques resp = excinfo.value assert resp.status_code == 400 - assert resp.status == ( - "400 Invalid value for classifiers. " - "Error: Classifier 'AA :: BB' has been deprecated, see /url " - "for a list of valid classifiers." - ) + assert resp.status == expected_message @pytest.mark.parametrize( "digests", diff --git a/warehouse/admin/static/js/controllers/deprecate_classifier_controller.js b/warehouse/admin/static/js/controllers/deprecate_classifier_controller.js new file mode 100644 index 000000000000..82698086c6cc --- /dev/null +++ b/warehouse/admin/static/js/controllers/deprecate_classifier_controller.js @@ -0,0 +1,40 @@ +/** + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + + +import { Controller } from "stimulus"; + +export default class extends Controller { + static targets = [ + "deprecatedClassifier", "alternativeClassifier" + ] + + update() { + const deprecatedClassifierId = this.deprecatedClassifierTarget.options[this.deprecatedClassifierTarget.selectedIndex].value; + this.alternativeClassifierTargets.forEach(target => { + const selectedAlternativeId = target.options[target.selectedIndex].value; + + // Reset the value to prevent self-selection. + if (deprecatedClassifierId === selectedAlternativeId) { + target.selectedIndex = 0; + } + + // Disable deprecated classifier selection. + for (let optionIndex = 0; optionIndex < target.options.length; ++optionIndex) { + const option = target.options[optionIndex]; + option.disabled = option.value === deprecatedClassifierId || option.dataset.deprecated; + } + }); + } +} diff --git a/warehouse/admin/templates/admin/classifiers/index.html b/warehouse/admin/templates/admin/classifiers/index.html index 08b30e100357..9508e67a4757 100644 --- a/warehouse/admin/templates/admin/classifiers/index.html +++ b/warehouse/admin/templates/admin/classifiers/index.html @@ -78,7 +78,7 @@

Add Sub-Classifier

-
+

Deprecate Classifier

@@ -87,7 +87,7 @@

Deprecate Classifier

- {% for classifier in classifiers %} {% endfor %} + + {% for deprecation_alternative_index in range(5) %} + + {% endfor %}
diff --git a/warehouse/admin/views/classifiers.py b/warehouse/admin/views/classifiers.py index 1d58cc71b969..4b68b669120e 100644 --- a/warehouse/admin/views/classifiers.py +++ b/warehouse/admin/views/classifiers.py @@ -92,14 +92,29 @@ def add_child_classifier(self): uses_session=True, require_methods=False, require_csrf=True, + request_param=["classifier_id"], ) def deprecate_classifier(request): - classifier = request.db.query(Classifier).get(request.params.get("classifier_id")) + deprecated_classifier_id = int(request.params.get("classifier_id")) + alternative_classifier_ids = { + int(alternative_classifier_id) + for alternative_classifier_id in request.params.getall("deprecated_by") + if alternative_classifier_id + } + + alternative_classifiers = [] + for alternative_classifier_id in alternative_classifier_ids: + alternative_classifiers.append( + request.db.query(Classifier).get(alternative_classifier_id) + ) - classifier.deprecated = True + deprecated_classifier = request.db.query(Classifier).get(deprecated_classifier_id) + deprecated_classifier.deprecated = True + for alternative_classifier in alternative_classifiers: + deprecated_classifier.alternatives.append(alternative_classifier) request.session.flash( - f"Deprecated classifier {classifier.classifier!r}", queue="success" + f"Deprecated classifier {deprecated_classifier.classifier!r}", queue="success" ) return HTTPSeeOther(request.route_path("admin.classifiers")) diff --git a/warehouse/classifiers/models.py b/warehouse/classifiers/models.py index 97615931f2cc..e5607342c8f8 100644 --- a/warehouse/classifiers/models.py +++ b/warehouse/classifiers/models.py @@ -10,12 +10,28 @@ # See the License for the specific language governing permissions and # limitations under the License. -from sqlalchemy import Boolean, Column, Integer, Text, sql +from sqlalchemy import Boolean, Column, ForeignKey, Integer, orm, Table, Text, sql from warehouse import db from warehouse.utils.attrs import make_repr +classification_deprecation_alternatives = Table( + "classification_deprecation_alternatives", + db.metadata, + Column( + "deprecated_classifier_id", + Integer, + ForeignKey("trove_classifiers.id", ondelete="CASCADE", onupdate="CASCADE"), + ), + Column( + "alternative_classifier_id", + Integer, + ForeignKey("trove_classifiers.id", ondelete="CASCADE", onupdate="CASCADE"), + ), +) + + class Classifier(db.ModelBase): __tablename__ = "trove_classifiers" @@ -29,3 +45,14 @@ class Classifier(db.ModelBase): l3 = Column(Integer) l4 = Column(Integer) l5 = Column(Integer) + + alternatives = orm.relationship( + "Classifier", + secondary=classification_deprecation_alternatives, + primaryjoin=( + classification_deprecation_alternatives.c.deprecated_classifier_id == id + ), + secondaryjoin=( + classification_deprecation_alternatives.c.alternative_classifier_id == id + ), + ) diff --git a/warehouse/forklift/legacy.py b/warehouse/forklift/legacy.py index 2411b62c1b06..9d40eb0430ee 100644 --- a/warehouse/forklift/legacy.py +++ b/warehouse/forklift/legacy.py @@ -13,6 +13,7 @@ import email import hashlib import hmac +import operator import os.path import re import tempfile @@ -681,15 +682,30 @@ def _no_deprecated_classifiers(request): def validate_no_deprecated_classifiers(form, field): invalid_classifiers = set(field.data or []) & deprecated_classifiers if invalid_classifiers: - first_invalid_classifier = sorted(invalid_classifiers)[0] + first_invalid_classifier = min(invalid_classifiers) host = request.registry.settings.get("warehouse.domain") classifiers_url = request.route_url("classifiers", _host=host) - - raise wtforms.validators.ValidationError( - f"Classifier {first_invalid_classifier!r} has been " - f"deprecated, see {classifiers_url} for a list of valid " - "classifiers." + alternatives = ( + request.db.query(Classifier) + .filter(Classifier.classifier == first_invalid_classifier) + .one() + .alternatives ) + alternatives.sort(key=operator.attrgetter("classifier")) + + if alternatives: + raise wtforms.validators.ValidationError( + f"Classifier {first_invalid_classifier!r} has been deprecated " + f"in favor of the following classifier(s): " + + ", ".join( + f"{classifier.classifier!r}" for classifier in alternatives + ) + ) + else: + raise wtforms.validators.ValidationError( + f"Classifier {first_invalid_classifier!r} has been deprecated, " + f"see {classifiers_url} for a list of valid classifiers." + ) return validate_no_deprecated_classifiers @@ -715,7 +731,7 @@ def file_upload(request): ) # Ensure that user has a verified, primary email address. This should both - # reduce the ease of spam account creation and activty, as well as act as + # reduce the ease of spam account creation and activity, as well as act as # a forcing function for https://github.com/pypa/warehouse/issues/3632. # TODO: Once https://github.com/pypa/warehouse/issues/3632 has been solved, # we might consider a different condition, possibly looking at diff --git a/warehouse/migrations/versions/0a392d8b1e7e_add_classifier_deprecation_alternatives_table.py b/warehouse/migrations/versions/0a392d8b1e7e_add_classifier_deprecation_alternatives_table.py new file mode 100644 index 000000000000..fd56db2f9def --- /dev/null +++ b/warehouse/migrations/versions/0a392d8b1e7e_add_classifier_deprecation_alternatives_table.py @@ -0,0 +1,49 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +""" +Add classification_deprecation_alternatives table + +Revision ID: 0a392d8b1e7e +Revises: e82c3a017d60 +Create Date: 2018-10-28 11:30:56.629863 +""" + +from alembic import op +import sqlalchemy as sa + + +revision = "0a392d8b1e7e" +down_revision = "e82c3a017d60" + + +def upgrade(): + op.create_table( + "classification_deprecation_alternatives", + sa.Column("deprecated_classifier_id", sa.Integer(), nullable=True), + sa.Column("alternative_classifier_id", sa.Integer(), nullable=True), + sa.ForeignKeyConstraint( + ["alternative_classifier_id"], + ["trove_classifiers.id"], + onupdate="CASCADE", + ondelete="CASCADE", + ), + sa.ForeignKeyConstraint( + ["deprecated_classifier_id"], + ["trove_classifiers.id"], + onupdate="CASCADE", + ondelete="CASCADE", + ), + ) + + +def downgrade(): + op.drop_table("classification_deprecation_alternatives")