[BUG FIX][Split Tool] Bug fix for split features tool #8035
Conversation
|
See https://issues.qgis.org/issues/19936 for some background |
|
test_SplitFeature test has failed =/ |
|
@phborba when the implementation is settled, you'll need to go through all the failing tests and carefully check/fix them manually. |
|
@nyalldawson can we have your eyes on this one also? |
|
Guys maybe this method of test_provider_postgres should change: Since the behaviour is altered. What do you think @elpaso , @m-kuhn and @nyalldawson ? |
|
@phborba yes, as soon as the adjusted behaviour is validated, the tests need to be adjusted to the new behavior as well. Feel free to do that already now, but in case there are still some other modifications required this will probably need to an extra effort. |
|
@phborba For the comment you are about to edit, I wonder if it's actually worth leaving this if it's adjusted. It will be no more than a historical consideration in the future. I'd rather explain what this does actually test after these changes, and maybe leave a minor note and a link to the issue if you want. |
| @@ -190,7 +190,7 @@ def choose_linked_feature(): | |||
| btn.click() | |||
| # magically the above code selects the feature here... | |||
|
|
|||
| link_feature = next(self.vl_link.getFeatures(QgsFeatureRequest().setFilterExpression('"fk_book"={}'.format(f[0])))) | |||
| link_feature = list(next(self.vl_link.getFeatures(QgsFeatureRequest().setFilterExpression('"fk_book"={}'.format(f[0]))))) | |||
There was a problem hiding this comment.
What's the rationale here? This has changed the test from testing the attribute index 0 to testing that the feature is not None (which will never be the case)
There was a problem hiding this comment.
Sometimes I get stop iteration error on python 3 due to the use of next because it now returns an iterator, not a list. Since I've seen this on code and travis has thrown a stop iteration error, I thought it would be nice to fix it. If you like me to, I can change it back to what was written! =]
There was a problem hiding this comment.
Ok, but in this case you have changed link_feature from a QgsFeature to a list of QgsFeature. So you'd need to make this
list(next(....))[0]
so that link_feature is the feature.
| @@ -258,14 +258,16 @@ def testCreateFeature(self): | |||
| # layer with default value expression | |||
| layer.setDefaultValueDefinition(2, QgsDefaultValue('3*4')) | |||
| f = QgsVectorLayerUtils.createFeature(layer) | |||
| self.assertEqual(f.attributes(), [NULL, NULL, 12.0]) | |||
| # we expect the default value expression to take precedence over the attribute map | |||
| self.assertEqual(f.attributes(), ['a', NULL, 6.0]) | |||
There was a problem hiding this comment.
I think this change is unintentional -- the values should not be affected by this bug fix
There was a problem hiding this comment.
Because of the new behavior, since we are using a value map as input, it should not be changed. Before, the default value had to overwrite the provided one, now the provided value must remain the same, don't you agree?
Sure @m-kuhn , I'll fix that! =] |
| f = QgsVectorLayerUtils.createFeature(layer, attributes={0: 'test_1', 1: 123}) | ||
| self.assertEqual(f.attributes(), ['test_4', 128, NULL]) | ||
| self.assertEqual(f.attributes(), [NULL, NULL, NULL]) |
There was a problem hiding this comment.
Not sure about that one. The output should be a new unique value instead of the conflicting one provided in attributes, no?
There was a problem hiding this comment.
In this particular data source there is no mechanism to auto increment a value, so the output is all NULLs.
I agree with you that it should happen when we are using data sources like postgresql.
When I ran this test locally, the only way of getting this check with this data source was to compare with [NULL, NULL, NULL]
There was a problem hiding this comment.
If we do not have yet a test with a PG layer with a sequence to get the new unique value from the provider it would be really nice to have it, so we could check if the value is correctly calculated.
There was a problem hiding this comment.
Interesting.
The current state (i.e. before this pull request) of the test suggests that there is a mechanism in place (attributes: test_1 result: test_4). Or am I misinterpreting something here?
There was a problem hiding this comment.
I must confess I was confused as well, but when I ran the tests locally I could see that it did not output as the code suggested. I agree with @elpaso , I'll study other QGIS' test code to add a createFeatures test from PG.
There was a problem hiding this comment.
There was a problem hiding this comment.
There was a problem hiding this comment.
Now I'm confused, because running the python test locally was ignoring the case you are mentioning.
Maybe I should test again on c++
There was a problem hiding this comment.
@m-kuhn and @elpaso I ran test case that we have doubts with the c++ debugger turned on and I have realised that the v=uniqueValue never is called on the last if (bellow)
if ( checkUnique && fields.at( idx ).constraints().constraints() & QgsFieldConstraints::ConstraintUnique )
{
if ( QgsVectorLayerUtils::valueExists( layer, idx, v ) )
{
// unique constraint violated
QVariant uniqueValue = QgsVectorLayerUtils::createUniqueValue( layer, idx, v );
if ( uniqueValue.isValid() )
v = uniqueValue;
}
}
To ensure this behaviour of this test (input: 'test_1' output: 'test_4') we must change the implementation. Maybe we should put an extra check inside the first if in order to create the unique value. I'm still working on that, if any of you guys have any suggestion, please feel free to share =]
There was a problem hiding this comment.
Changed the first if to:
if ( attributes.contains( idx ) )
{
v = attributes.value( idx );
if ( fields.at( idx ).constraints().constraints() & QgsFieldConstraints::ConstraintUnique
&& QgsVectorLayerUtils::valueExists( layer, idx, v ) )
{
// unique constraint violated
QVariant uniqueValue = QgsVectorLayerUtils::createUniqueValue( layer, idx, v );
if ( uniqueValue.isValid() )
v = uniqueValue;
}
checkUnique = false;
}
And changed back the tests you have pointed out.
| self.assertEqual(f.attributes(), [NULL, NULL, 300.0]) | ||
| layer.setDefaultValueDefinition(2, QgsDefaultValue(None)) | ||
|
|
||
| # test with violated unique constraints | ||
| layer.setFieldConstraint(1, QgsFieldConstraints.ConstraintUnique) | ||
| f = QgsVectorLayerUtils.createFeature(layer, attributes={0: 'test_1', 1: 123}) | ||
| self.assertEqual(f.attributes(), ['test_1', 128, NULL]) | ||
| # since field 1 has Unique Constraint, it ignores value 123 that already has been set | ||
| self.assertEqual(f.attributes(), ['test_1', NULL, NULL]) |
There was a problem hiding this comment.
This should remain 128 for attribute 1
There was a problem hiding this comment.
You are right, I messed up there, so I changed the test back to the way it was and changed the c++ code.
| layer.setFieldConstraint(0, QgsFieldConstraints.ConstraintUnique) | ||
| # since field 0 and 1 already have values test_1 and 123, the output must be NULL |
There was a problem hiding this comment.
the output must be NULL -> the output must be a new unique value
There was a problem hiding this comment.
You are right, I messed up there, so I changed the test back to the way it was and changed the c++ code.
|
Looks good to me! @m-kuhn? |
|
@nyalldawson sorry about all the trouble in my commits, it's the first time I contribute to QGIS project and I was very afraid of messing things up! @nyalldawson , @m-kuhn and @elpaso thanks for the help and tips! I hope I can help you guys on other occasions, I value a lot all the work and effort you guys put into QGIS. We at the Brazilian Army are massively using QGIS and we would not be able to use QGIS here if not for all the contributions you and a lot of other guys give to the project. |
|
@phborba no need to apologise - instead just bask in our gratitude for a great fix! There's a big learning curve when it comes to qgis development, so it's natural that it takes some time to get your head around the code. Bring on the next fix! ;) |
|
Yeah! Thank you @phborba for the fix! |
Description
Split feature was resetting attributes of new features with PostGIS providers that have default values.
For example, if a layer has a field called field1 and it has a default value of 1, if a user used the split tool in a feature that has field1=2, the new part instead of getting the old value of 2 gets the provider value of 1.
A small change on createFeature from QgsVectorLayerUtils was made.
This bugfix is a suggestion for Bug report #19936 from redmine.
Checklist
fixes #11111in the commit message next to the description[FEATURE]in the commit message[needs-docs]in the commit message and contain sufficient information in the commit message to be documentedscripts/prepare-commit.shscript before each commit