[patch] Reject uploads that do not follow either comment policy or source policy

[ljdelight]

The upload checks on the comment/source policy needs a slight tweak since an invalid 'comment' and valid 'source' are allowed to be uploaded (either case being invalid should stop an upload).

This patch checks that both (the comment and source) comments are valid before allowing an upload.

[simon04]

Sorry, I don't see how this patch makes a difference. Since UploadCommentValidator/UploadSourceValidator both extend UploadTextComponentValidator, they never share the boolean uploadRejected field.

Please add a unit test to org.openstreetmap.josm.gui.io.UploadTextComponentValidatorTest showing where your patch makes a difference.

[ljdelight]

Slow reply, sorry about that, @simon04 can we re-open this ticket? I'll make a new patch and digging in to why the UploadTextComponentValidator.isUploadRejected is returning false in a specific case

[ljdelight]

Fixed

[ljdelight]

My initial patch makes no sense, you're absolutely right @simon04 and I feel silly :-)

The latest patch fixes the issue.
There were two problems causing the upload button to be selectable even though the comment/source tag policy was not met:

• The input length was validated first and took priority over the tag policy. This is resolved by checking the tag policy and, if that's valid, then we check the input length.
• The upload button state is modified by callbacks which check the tag policy, however the initial state of the button doesn't account for the tag policy.

To reproduce the issue:

1. In the advanced settings set a comment or source tag policy (eg upload.comment.mandatory-terms)
2. Download a small map area and make a minor change to a map (eg move a node)
3. Click the Upload button
4. The Upload Changes button is selectable. This should not be the case.
5. Type at least 11 characters to trigger the update events of the dialog which check the tag policy
6. The Upload Changes button is now not selectable

[ljdelight]

Hi @simon04, any updates you need from me here? Thanks

[ljdelight]

Reassigning owner from me to 'team', that seems to match other tickets

[taylor.smock]

The functionality seems to work. Would you mind (if you are still around) writing tests, as simon04 said in comment:2. That way we will notice if it is accidentally broken.

[ljdelight]

Rebased to trunk and added unit test for this case

[ljdelight]

Hi Taylor, I added patch-20823-3.patch and it contains changes that work against trunk and includes unit tests.

[taylor.smock]

It looks good to me. I'll go ahead and merge this on Monday to give other maintainers a chance to look at it.

If I forget to merge this on Monday (UTC-6), feel free to ping me.

Anyway, I'm sorry that your patch has been forgotten for ~1 year. I've been trying to go through tickets with patches attached (see report:8), but I've been mostly focused on (a) new tickets and (b) recent patches.

[taylor.smock]

In 18491/josm:

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.