lib/model: Support removing paused folders (fixes #4357)

[AudriusButkevicius]

Tested manually.

[imsodin]

I think this should never happen and thus panic

[AudriusButkevicius]

When/if we add HTTP DELETE /folders/xyz API, this would make sense.

[imsodin]

Shouldn't that result in a returned error? I think there are some endpoints presently that check whether a folder is present before doing anything.

[imsodin]

Test:

func TestRemovePausedFolder(t *testing.T) {
	// issue 4357

	db := db.OpenMemory()
	m := NewModel(defaultConfig, protocol.LocalDeviceID, "syncthing", "dev", db, nil)
	defCfg := defaultFolderConfig.Copy()
	m.AddFolder(defCfg)
	m.StartFolder("default")
	m.ServeBackground()
	defer m.Stop()

	oldCfg := m.cfg.RawCopy()
	newCfg := oldCfg.Copy()
	newCfg.Folders[0].Paused = true
	m.CommitConfiguration(oldCfg, newCfg)

	m.RemoveFolder("default")
	m.fmut.Lock()
	if _, ok := m.folderCfgs["default"]; ok {
		t.Error("Paused folder was not removed")
	}
	m.fmut.Unlock()
}

[AudriusButkevicius]

I think the test is not very useful. CommitConfig does the removal from folderCfgs, not the m.RemoveFolder in this case.

[AudriusButkevicius]

Yet let me add a different one, and add the panic I guess.

[imsodin]

I think the test is not very useful. CommitConfig does the removal from folderCfgs, not the m.RemoveFolder in this case.

It panics as in the referenced issue, so I guess it does what it is supposed to: reproduce it - what am I missing?

[AudriusButkevicius]

My point was that this check was too late:

if _, ok := m.folderCfgs["default"]; ok {
	t.Error("Paused folder was not removed")
}

Anyways, updated and added a test case.

[imsodin]

Ups, wrong bot.

@st-release lgtm

[imsodin]

@st-review lgtm

[st-review]

@imsodin: Noted! Need another LGTM or explicit merge command.

[calmh]

@st-review lgtm

[st-review]

👌 Merged as e85ce7c. Thanks, @AudriusButkevicius!