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.

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

fixes #13504 : Update layer statistics when adding column to table in order to be displayed by QGIS #8853

Merged
merged 4 commits into from
Jan 21, 2019

Conversation

troopa81
Copy link
Contributor

Description

When adding a column from the DB manager, new column is not displayed in QGIS. We have to recompute layer statistics : https://www.gaia-gis.it/fossil/libspatialite/wiki?name=4.2.0+functions 4 - Forcing Layer Statistics to be recalculated

This PR corrects this issue.

I didn't find existing unit tests for this parts. Does plugin needs tests?

Checklist

  • Commit messages are descriptive and explain the rationale for changes
  • Commits which fix bugs include fixes #11111 in the commit message next to the description
  • Commits which add new features are tagged with [FEATURE] in the commit message
  • Commits which change the UI or existing user workflows are tagged with [needs-docs] in the commit message and contain sufficient information in the commit message to be documented
  • I have read the QGIS Coding Standards and this PR complies with them
  • This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
  • New unit tests have been added for core changes
  • I have run the scripts/prepare-commit.sh script before each commit

@elpaso
Copy link
Contributor

elpaso commented Jan 14, 2019

@troopa81 I don't think there are tests for DB Manager (an this is an endless source of problems).

It would be great if you could start adding some tests for DB manager, but of course it is not required.

@nyalldawson
Copy link
Collaborator

There's https://github.com/qgis/QGIS/blob/master/tests/src/python/test_db_manager_gpkg.py -- a similar test file could be added for spatialite

@nyalldawson nyalldawson added Requires Tests! Waiting on the submitter to add unit tests before eligible for merging Bugfix Needs Backporting labels Jan 14, 2019
@troopa81
Copy link
Contributor Author

I add some unit tests based on gpkg ones. Actually, I just copied the test_db_manager_gpkg.py and made some modifications to make it works.

But maybe it should be better to keep only one file with some parameters (some methods are not implemented in spatialite, lower/uppercase is not well managed, gdal table info results is different) instead of maintening two files.

Maybe we could you use data driven test https://ddt.readthedocs.io/en/latest/example.html

@nyalldawson
Copy link
Collaborator

Thanks for the new test! You'll need to add it to https://github.com/qgis/QGIS/blob/master/tests/src/python/CMakeLists.txt in order for it to run

@troopa81
Copy link
Contributor Author

@nyalldawson I can add the test but would not be better to keep only one file with parameters using ddt, pytest (https://docs.pytest.org/en/latest/parametrize.html), or even for loop (*for driver in ["SQLite", "gpkg"] ... *) like it's done in other tests?

@nyalldawson
Copy link
Collaborator

@troopa81 Personally I'm OK with the duplicate code in the new test here. I think there's going to be so many behavioural differences between the different db manager backends that the individual backend tests will end up looking quite different from each other anyway. And I'd rather get this fix in ASAP and leave test refactoring to a potential follow up PR.

Can you make the required change in https://github.com/qgis/QGIS/blob/master/tests/src/python/CMakeLists.txt so that we can merge this fix?

@troopa81
Copy link
Contributor Author

@nyalldawson OK, I understand. I just made the modification.


def deleteTableColumn(self, table, column):
""" delete column from a table """

print("test")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Drop this line

@nyalldawson nyalldawson removed the Requires Tests! Waiting on the submitter to add unit tests before eligible for merging label Jan 18, 2019
@nyalldawson nyalldawson merged commit 49cb397 into qgis:master Jan 21, 2019
@nyalldawson
Copy link
Collaborator

Thanks, merged! I'll backport to 3.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants