[RFC Patch] Test-Failure because of r18994,r18996

[stoecker]

Currently tests fail because of my changes to the save dialog.

@Taylor: I'm not 100% sure, but I think the test is probably wrong or tuned to bad behaviour? What do you think?

[taylor.smock]

I assume you are talking about the non-regression test for #22817.
I think the test is probably partly wrong. Specifically BLOCKED files don't have a way to become blocked via the UI, so they must be loaded from disk is wrong (I forgot about remote control).
With that said, I think the test as written is currently fine. We do want to nag the user if the layer is BLOCKED and has a backing file.
The failing test is DISCOURAGED though. Not BLOCKED.

So, to clarify, we want the user to be nagged to save in the following cases:

• Layer is modifiable AND modified AND upload policy is NORMAL
• Layer is modifiable AND modified AND upload policy is DISCOURAGED (reminder: user can set the discouraged flag via UI)
• Layer is modifiable AND modified AND upload policy is NEVER AND layer has backing file

We do not want to nag the user to save in the following circumstances

• Layer is not modifiable
• Layer is not modified
• Layer has upload policy of NEVER AND has no backing file

EDIT: The problematic layers are usually from HOTOSM. The remote control command they use is as follows:
http://127.0.0.1:8111/import?new_layer=true&layer_name=Boundary for task: 386 of TM Project #16178 - Do not edit or upload&layer_locked=true&download_policy=never&upload_policy=never&url=https://tasking-manager-tm4-production-api.hotosm.org/api/v2/projects/16178/tasks/queries/xml/?tasks=386

• layer_locked=true
• upload_policy=never

[taylor.smock]

Maybe

src/org/openstreetmap/josm/gui/io/SaveLayersDialog.java

@@ -123 +123 @@
                 AbstractModifiableLayer odl = (AbstractModifiableLayer) l;
                 if (odl.isModified() && (
                         (odl.isSavable() && odl.requiresSaveToFile()) ||
-                        (odl.isUploadable() && odl.requiresUploadToServer() && !odl.isUploadDiscouraged()))) {
+                        (odl.isUploadable() && odl.requiresUploadToServer()))) {
                     layersWithUnsavedChanges.add(odl);
                 }
             }

isUploadable is data.getUploadPolicy() != UploadPolicy.BLOCKED && !isLocked().

[stoecker]

Replying to taylor.smock:

I assume you are talking about the non-regression test for #22817.

Jupp.

I think the test is probably partly wrong. Specifically BLOCKED files don't have a way to become blocked via the UI, so they must be loaded from disk is wrong (I forgot about remote control).

Long text, I have to think more about it. Didn't think that's so complex. ;-)

With that said, I think the test as written is currently fine. We do want to nag the user if the layer is BLOCKED and has a backing file.
The failing test is DISCOURAGED though. Not BLOCKED.

So, to clarify, we want the user to be nagged to save in the following cases:

• Layer is modifiable AND modified AND upload policy is NORMAL

Yes. But why "nagged to save"? Do you mean "nagged to upload"?

• Layer is modifiable AND modified AND upload policy is DISCOURAGED (reminder: user can set the discouraged flag via UI)
• Layer is modifiable AND modified AND upload policy is NEVER AND layer has backing file

Regarding upload? Why do you want to nag about upload for non-uploadable layers? Or do you mean saving them?

We do not want to nag the user to save in the following circumstances

• Layer is not modifiable

Yes.

• Layer is not modified

Yes.

• Layer has upload policy of NEVER AND has no backing file

What has upload policy to do with saving?

EDIT: The problematic layers are usually from HOTOSM. The remote control command they use is as follows:
http://127.0.0.1:8111/import?new_layer=true&layer_name=Boundary for task: 386 of TM Project #16178 - Do not edit or upload&layer_locked=true&download_policy=never&upload_policy=never&url=https://tasking-manager-tm4-production-api.hotosm.org/api/v2/projects/16178/tasks/queries/xml/?tasks=386

• layer_locked=true
• upload_policy=never

I viewed the save and upload as to different decisions only presented in one dialog.

I think your patch isn't right either, as it will again nag about my local file with upload=false/never which is not meant for uploading. It will open a dialog which offers save, but save is unchecked by default. That's the reason why I added that additional check. ATM I only get the dialog when I change the file: That's what I expect.

[stoecker]

Maybe we should drop the double logic altogether, simply construct the dialog and don't show it when none of the buttons is checked by default?

[GerdP]

The test iterates over the different upload strategies, so it should not test wether the data is savable.

[GerdP]

possible fix

[GerdP]

The patch changes the failing test so that it

• doesn't use a new file but reads from an existing one so that the result doesn't depend on that part
• tests more methods to show the differences between DataSet.requiresUploadToServer() and OsmDataLayer.requiresUploadToServer()
• tests the effect of the upload policy on the SaveLayersDialog

[GerdP]

I think the code in core is correct, just the unit test has to be fixed. Any complains about the attached patch?

[taylor.smock]

I don't see anything objectionable in the patch.

[GerdP]

In 19011/josm:

fix #23509: Test-Failure because of r18994,r18996
The patch changes the failing test so that it

• doesn't use a new file but reads from an existing one so that the result doesn't depend on that part
• tests more methods to show the differences between DataSet.requiresUploadToServer() and OsmDataLayer.requiresUploadToServer()
• tests the effect of the upload policy on the SaveLayersDialog