Add default ServerGroup to user loading 0 Servers

[benjaminjb]

• check for 0 ServerGroups as result of uploading empty Servers list and re-add the default ServerGroup
• remove duplicate Server delete loop

Fixes #6602

[khushboovashi]

The changes made in load_database_servers function doesn't make sense.
If any user cleared all the servers through command line and then wants to add server from the UI, then this fix will never be executed.

You should add this change in clear_database_servers itself.

[benjaminjb]

The changes made in load_database_servers function doesn't make sense. If any user cleared all the servers through command line and then wants to add server from the UI, then this fix will never be executed.

You should add this change in clear_database_servers itself.

I've only started digging into the code, so happy to adjust, but just wanted to check:

a. does clear_database_servers ever run independently? It looks to me like the only way to call clear_database_servers is to call an import function with the replace option:

-- from the setup.py script here
-- from the web import function here

Is there some way to clear the servers from the commandline without importing?

b. I do like the idea of putting a check like this in clear_database_servers because if clear_database_servers runs, but the import fouls up for some reason, the user will be left in a bad state for manual creation, like you said.

BUT if we clear the servers from a user, then add a default Servers ServerGroup back, then import the user's JSON file, the user will then end up with (1) the servers they imported and (b) an un-asked for default Servers ServerGroup. That feels like a slightly odd UX.

Another option would be to clear user servers & create a default in clear_database_servers; then load the servers & potentially remove the default Servers in load_database_servers -- but that starts to feel a little fiddly.

Happy to make requested changes, but just want to clarify the expected results.

[khushboovashi]

The changes made in load_database_servers function doesn't make sense. If any user cleared all the servers through command line and then wants to add server from the UI, then this fix will never be executed.
You should add this change in clear_database_servers itself.

I've only started digging into the code, so happy to adjust, but just wanted to check:

a. does clear_database_servers ever run independently? It looks to me like the only way to call clear_database_servers is to call an import function with the replace option:

-- from the setup.py script here -- from the web import function here

Is there some way to clear the servers from the commandline without importing?

/path/to/python /path/to/setup.py --load-servers input_file.json --replace

Import/Export server functionality has UI and CLI based both the implementation.
Ref: https://www.pgadmin.org/docs/pgadmin4/7.7/import_export_servers.html#importing-servers

b. I do like the idea of putting a check like this in clear_database_servers because if clear_database_servers runs, but the import fouls up for some reason, the user will be left in a bad state for manual creation, like you said.

BUT if we clear the servers from a user, then add a default Servers ServerGroup back, then import the user's JSON file, the user will then end up with (1) the servers they imported and (b) an un-asked for default Servers ServerGroup. That feels like a slightly odd UX.

Agreed.

Another option would be to clear user servers & create a default in clear_database_servers; then load the servers & potentially remove the default Servers in load_database_servers -- but that starts to feel a little fiddly.

Happy to make requested changes, but just want to clarify the expected results.

As per the current implementation, pgAdmin does not allow the default server-group to be deleted from the left tree browser.
So, that should be implemented in the clear_database_servers function and in that way both your concerns should be taken care of.

Please let me know if you still have any doubt.

[benjaminjb]

Thx for triggering the tests, @khushboovashi !

Looking at the failures briefly this morning, looks like

1. the run-python-tests-pg workflow is failing in Install Python dependencies on Windows in 5 of 5 tests with Failed to build psycopg-c
2. the run-feature-tests-pg workflow for PG 12 failed the CheckFileManagerFeatureTest

Failure 1 seems like something beyond the scope of this PR, though I didn't see other recent-ish (2-3 days old) PRs failing that.
For Failure 2, I browsed a little to see why this test failed, but wasn't sure. It doesn't seem like it's particularly flaky, but now sure.