Changeset 18491 in josm

Timestamp:

2022-06-13T23:40:17+02:00 (22 months ago)

Author:

taylor.smock

Message:

Fix #20823: Reject uploads that do not follow either comment policy or source policy (patch by ljdelight)

This fixes an issue where a the length of the comment or source was checked first,
and a warning instead of a rejection was generated when the upload policy would
have otherwise rejected the comment or source.

This also performs the validation check first, prior to the user being able to
make changes in the upload dialog UI, so that the initial dialog state matches
the upload policy.

Location:

trunk

Files:

• src/org/openstreetmap/josm/gui/io/UploadDialog.java (modified) (1 diff)
• src/org/openstreetmap/josm/gui/io/UploadTextComponentValidator.java (modified) (4 diffs)
• test/unit/org/openstreetmap/josm/gui/io/UploadTextComponentValidatorTest.java (modified) (2 diffs)
• test/unit/org/openstreetmap/josm/testutils/annotations/BasicPreferences.java (modified) (1 diff)

trunk/src/org/openstreetmap/josm/gui/io/UploadDialog.java

@@ -222 +222 @@
         );
 
+        // Set the initial state of the upload button
+        btnUpload.setEnabled(pnlBasicUploadSettings.getUploadTextValidators()
+                .stream().noneMatch(UploadTextComponentValidator::isUploadRejected));
+
         // Enable/disable the upload button if at least an upload validator rejects upload
         pnlBasicUploadSettings.getUploadTextValidators().forEach(v -> v.addChangeListener(e -> btnUpload.setEnabled(

trunk/src/org/openstreetmap/josm/gui/io/UploadTextComponentValidator.java

@@ -84 +84 @@
                 return;
             }
-            String uploadComment = getComponent().getText();
-            if (UploadDialog.UploadAction.isUploadCommentTooShort(uploadComment)) {
+            final String uploadComment = getComponent().getText();
+            final String rejection = UploadDialog.UploadAction.validateUploadTag(uploadComment, "upload.comment",
+                    Collections.emptyList(), Collections.emptyList(), Collections.emptyList());
+
+            // Reject the upload if tags are required and are not in the input. If the tags exist or are not
+            // required, then check the length of the input and warn if it's too short (a short msg is not a rejection)
+            uploadRejected = rejection != null;
+
+            if (uploadRejected) {
+                feedbackWarning(tr("Your upload comment is <i>rejected</i>.") + "<br />" + rejection);
+            } else if (UploadDialog.UploadAction.isUploadCommentTooShort(uploadComment)) {
                 feedbackWarning(tr("Your upload comment is <i>empty</i>, or <i>very short</i>.<br /><br />" +
                         "This is technically allowed, but please consider that many users who are<br />" +
@@ -93 +102 @@
                         "easier for many other mappers.").replace("<br />", " "));
             } else {
-                String rejection = UploadDialog.UploadAction.validateUploadTag(uploadComment, "upload.comment",
-                        Collections.emptyList(), Collections.emptyList(), Collections.emptyList());
-                uploadRejected = rejection != null;
-                if (uploadRejected) {
-                    feedbackWarning(tr("Your upload comment is <i>rejected</i>.") + "<br />" + rejection);
-                } else {
-                    feedbackValid(tr("Thank you for providing a changeset comment! " +
-                            "This gives other mappers a better understanding of your intent."));
-                }
+                feedbackValid(tr("Thank you for providing a changeset comment! " +
+                        "This gives other mappers a better understanding of your intent."));
             }
         }
@@ -121 +123 @@
                 return;
             }
-            String uploadSource = getComponent().getText();
-            if (Utils.isStripEmpty(uploadSource)) {
+            final String uploadSource = getComponent().getText();
+            final String rejection = UploadDialog.UploadAction.validateUploadTag(
+                    uploadSource, "upload.source", Collections.emptyList(), Collections.emptyList(), Collections.emptyList());
+
+            // Reject the upload only if tags are required and are not in the input. If the tags exist or are not
+            // required, then check the length of the input and warn if it's too short (a short msg is not a rejection)
+            uploadRejected = rejection != null;
+
+            if (uploadRejected) {
+                feedbackWarning(tr("Your changeset source is <i>rejected</i>.") + "<br />" + rejection);
+            } else if (Utils.isStripEmpty(uploadSource)) {
                 feedbackWarning(tr("You did not specify a source for your changes.<br />" +
                         "It is technically allowed, but this information helps<br />" +
@@ -129 +140 @@
                         "easier for many other mappers.").replace("<br />", " "));
             } else {
-                final String rejection = UploadDialog.UploadAction.validateUploadTag(
-                        uploadSource, "upload.source", Collections.emptyList(), Collections.emptyList(), Collections.emptyList());
-                uploadRejected = rejection != null;
-                if (uploadRejected) {
-                    feedbackWarning(tr("Your changeset source is <i>rejected</i>.") + "<br />" + rejection);
-                } else {
-                    feedbackValid(tr("Thank you for providing the data source!"));
-                }
+                feedbackValid(tr("Thank you for providing the data source!"));
             }
         }

trunk/test/unit/org/openstreetmap/josm/gui/io/UploadTextComponentValidatorTest.java

@@ -5 +5 @@
 import static org.hamcrest.MatcherAssert.assertThat;
 
+import java.util.Arrays;
+import java.util.function.BiFunction;
+import java.util.stream.Stream;
+
 import javax.swing.JLabel;
 import javax.swing.JTextField;
 
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import org.openstreetmap.josm.spi.preferences.Config;
 import org.openstreetmap.josm.testutils.annotations.BasicPreferences;
 
@@ -40 +48 @@
         assertThat(feedback.getText(), containsString("Thank you for providing the data source"));
     }
+
+    static Stream<Arguments> testUploadWithMandatoryTerm() {
+        return Stream.of(Arguments.of("upload.comment.mandatory-terms", "Thank you for providing a changeset comment",
+                    (BiFunction<JTextField, JLabel, ? extends UploadTextComponentValidator>)
+                            UploadTextComponentValidator.UploadCommentValidator::new),
+                Arguments.of("upload.source.mandatory-terms", "Thank you for providing the data source",
+                    (BiFunction<JTextField, JLabel, ? extends UploadTextComponentValidator>)
+                            UploadTextComponentValidator.UploadSourceValidator::new)
+        );
+    }
+
+    /**
+     * Unit test of {@link UploadTextComponentValidator.UploadCommentValidator} and
+     * {@link UploadTextComponentValidator.UploadSourceValidator} with mandatory terms
+     */
+    @BasicPreferences
+    @ParameterizedTest
+    @MethodSource
+    void testUploadWithMandatoryTerm(String confPref, String expectedText,
+            BiFunction<JTextField, JLabel, ? extends UploadTextComponentValidator> validatorSupplier) {
+        Config.getPref().putList(confPref, Arrays.asList("myrequired", "xyz"));
+        JTextField textField = new JTextField("");
+        JLabel feedback = new JLabel();
+
+        validatorSupplier.apply(textField, feedback);
+
+        // A too-short string should fail validation
+        textField.setText("");
+        assertThat(feedback.getText(), containsString("The following required terms are missing: [myrequired, xyz]"));
+
+        // A long enough string without the mandatory terms should claim that the required terms are missing
+        textField.setText("a string long enough but missing the mandatory term");
+        assertThat(feedback.getText(), containsString("The following required terms are missing: [myrequired, xyz]"));
+
+        // A valid string should pass
+        textField.setText("a string long enough with the mandatory term #myrequired #xyz");
+        assertThat(feedback.getText(), containsString(expectedText));
+    }
 }

trunk/test/unit/org/openstreetmap/josm/testutils/annotations/BasicPreferences.java

@@ -57 +57 @@
             // Disable saving on put, just to avoid overwriting pref files
             pref.enableSaveOnPut(false);
+            pref.resetToDefault();
             Config.setPreferencesInstance(pref);
             Config.setBaseDirectoriesProvider(JosmBaseDirectories.getInstance());