Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

MS SQL connection details #8780

Merged
merged 1 commit into from
Jan 23, 2024
Merged

MS SQL connection details #8780

merged 1 commit into from
Jan 23, 2024

Conversation

selmaVH1
Copy link
Collaborator

Fixes #6443
Fixes #6644
Fixes #3063

@selmaVH1 selmaVH1 merged commit 5a4ee00 into qgis:master Jan 23, 2024
DelazJ
DelazJ reviewed Jan 23, 2024
View reviewed changes
Copy link
Collaborator

@DelazJ DelazJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still publish my unfnished review

docs/user_manual/managing_data_source/opening_data.rst
geometry column attached will also be shown in the available table list.
* |checkbox| :guilabel:`Use estimated table parameters` only estimated table
metadata will be used. This avoids a slow table scan, but may result in
incorrect layer properties such as layer extant.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
incorrect layer properties such as layer extant.
incorrect layer properties such as layer extent.

docs/user_manual/managing_data_source/opening_data.rst
* |checkbox| :guilabel:`Use primary key from geometry_columns table`
* |checkbox| :guilabel:`Also list table with no geometry` tables without a
geometry column attached will also be shown in the available table list.
* |checkbox| :guilabel:`Use estimated table parameters` only estimated table
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* |checkbox| :guilabel:`Use estimated table parameters` only estimated table
* |checkbox| :guilabel:`Use estimated table parameters`: only estimated table

docs/user_manual/managing_data_source/opening_data.rst
this checkbox is dependent on the first one; it remains disabled unless
the first option is checked.
* |checkbox| :guilabel:`Use primary key from geometry_columns table`
* |checkbox| :guilabel:`Also list table with no geometry` tables without a
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* |checkbox| :guilabel:`Also list table with no geometry` tables without a
* |checkbox| :guilabel:`Also list table with no geometry`: tables without a

docs/user_manual/managing_data_source/opening_data.rst
* |checkbox| :guilabel:`Use estimated table parameters` only estimated table
metadata will be used. This avoids a slow table scan, but may result in
incorrect layer properties such as layer extant.
* |checkbox| :guilabel:`Skip invalid geometry handling` all handling of records
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* |checkbox| :guilabel:`Skip invalid geometry handling` all handling of records
* |checkbox| :guilabel:`Skip invalid geometry handling`: all handling of records

docs/user_manual/managing_data_source/opening_data.rst
incorrect layer properties such as layer extant.
* |checkbox| :guilabel:`Skip invalid geometry handling` all handling of records
with invalid geometry will be disabled. This speeds up the provider, however,
if any invalid geometries are present in table then the result is unpredictable
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if any invalid geometries are present in table then the result is unpredictable
if any invalid geometries are present in a table then the result is unpredictable

docs/user_manual/managing_data_source/opening_data.rst
all geometries present in the database are valid, and any newly added geometries
or tables will also be valid.
* |checkbox| :guilabel:`Use only a Subset of Schemas` will alow you to filter
schemas for MS SQL connection. If checked, only selected schemas will be displayed.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
schemas for MS SQL connection. If checked, only selected schemas will be displayed.
schemas for MS SQL connection. If enabled, only checked schemas will be displayed.

docs/user_manual/managing_data_source/opening_data.rst
or tables will also be valid.
* |checkbox| :guilabel:`Use only a Subset of Schemas` will alow you to filter
schemas for MS SQL connection. If checked, only selected schemas will be displayed.
You can check or uncheck al schemas in the MS SQL connection widget.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
You can check or uncheck al schemas in the MS SQL connection widget.
You can right-click to :guilabel:`Check` or :guilabel:`Uncheck` any schema in the list.

docs/user_manual/managing_data_source/opening_data.rst
Comment on lines +1092 to +1093
After you connect to MS SQL Server, in the :guilabel:`Database Details`
you are able to display databases by clicking on the :guilabel:`List Databases`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That connection dialog is quite messy and uncommon in QGIS so as I do not have access to mssql db, it is unclear to me how it works.

After you connect to MS SQL Server,

Is providing the above information that triggers the connection? Or is that connection done when you actually press "List databases" (and btw displays the list)? I'm able to hit the "list DB" button without any of those info. Would it be smthg like

Suggested change
After you connect to MS SQL Server, in the :guilabel:`Database Details`
you are able to display databases by clicking on the :guilabel:`List Databases`.
In the :guilabel:`Database Details` section, press the :guilabel:`List Databases` button
to connect to the MS SQL server and display available databases.

Or (worse), you have to leave that dialog, hit the connect button in the DSM dialog and then come back to hit the "list db" tools and the other options? And do you know what is the purpose of the "Test connection" button then?

docs/user_manual/managing_data_source/opening_data.rst
After you connect to MS SQL Server, in the :guilabel:`Database Details`
you are able to display databases by clicking on the :guilabel:`List Databases`.

Optionally, you can activate the following checkboxes:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Optionally, you can activate the following checkboxes:
Optionally, you can activate the following options:

@selmaVH1
Copy link
Collaborator Author

I still publish my unfnished review

Sorry I didn't know that you were preparing a review. I will repeat the entire process so that I can address your questions and create a new PR for the suggested changes.

@DelazJ
Copy link
Collaborator

DelazJ commented Jan 23, 2024

Sorry I didn't know that you were preparing a review.

No worries. I've been busy with other projects and some backend issues here so did not have much time for reviewing and publishing in a row.
spoiler: your other PRs are also under commenting 😉

@DelazJ DelazJ added the backport release_3.34 On merge create a backported pull request to 3.34 label Jan 23, 2024
@qgis-bot qgis-bot mentioned this pull request Jan 23, 2024
Merged
@selmaVH1 selmaVH1 mentioned this pull request Jan 26, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DelazJ DelazJ DelazJ left review comments

Assignees
No one assigned
Labels
backport release_3.34 On merge create a backported pull request to 3.34
Projects
None yet
Milestone
No milestone
2 participants
@selmaVH1 @DelazJ