fix: sample attribute lat lon regex #2036
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alli83
force-pushed
the
fix-2022-sample-attribute-lat-lon-regex-and-migration
branch
from
65c8283 to
85d93a8
Compare
alli83
force-pushed
the
fix-2022-sample-attribute-lat-lon-regex-and-migration
branch
from
85d93a8 to
f25ddc3
Compare
kencho51
reviewed
Nov 22, 2024
Comment on lines
336
to
339
| if(preg_match('/^[^0-9]*$/', $attributeData[1])) { | ||
| $errorMessage[] = sprintf('Attribute value for %s doesn\'t match WGS84 decimal format. | ||
| For geographic location (country, sea, region) use another attribute name', $attributeData[0]); | ||
| } |
There was a problem hiding this comment.
question: Can specify the actual attribute name for the geographic location (country, sea, region) in the warning message? And usually the geographic location (country, sea, region) is something like, Guangzhou, China, London, UK`, the regex seems not really applicable to this case?
There was a problem hiding this comment.
the goal here is to check if they intend to use the attribute 391 and 392. If there are no numbers in the value, we will inform them that it seems like they may have chosen the wrong attribute. If there are numbers, we will check if it matches the WGS84 format. If not, we will indicate that there will likely be an error. It's been asked not to block the curator and just display a warning message.
kencho51
reviewed
Nov 22, 2024
| </div> | ||
| <div class='modal-footer'> | ||
| <a id="hideModal" class='btn background-btn-o'>Cancel</a> | ||
| <?php echo CHtml::submitButton('Confirm', array('class' => 'btn background-btn')); ?> |
There was a problem hiding this comment.
question: If the invalid geolocation was allowed to be overrided and saved, what is the point of having the validation rule?
suggestion: My suggestion is not allow user to save any invalid value.
There was a problem hiding this comment.
I agree with you but it has been requested to make it just a warning message that can be overriden
There was a problem hiding this comment.
As a user (and product owner) I think we should set it as a hard rule with no over-ride. It wont stop errors in format coming in via the excel spreadsheet upload but it will make sure curators fix them if they try to edit the sample metadata. As long as its set to only be applied to the correct attribute ID's (391 and 392).
There was a problem hiding this comment.
I have conflicting information regarding whether to block the action or just display a warning message
kencho51
reviewed
Nov 22, 2024
tests/acceptance/AdminSample.feature
Outdated
Comment on lines
119
to
202
| Scenario: display 1 warning message when create with wrong latitude beyond 90 | ||
| Given I am on "/adminSample/create" | ||
| And I should see "Create" | ||
| When I fill in the field of "name" "Sample[species_id]" with "87676:Eucalyptus pauciflora" | ||
| And I fill in the field of "name" "Sample[attributesList]" with "latitude=\"95.1234\"" | ||
| And I press the button "Create" | ||
| And I wait "1" seconds | ||
| Then I should see "Are you sure you want to continue?" | ||
| And I should see "Attribute value for latitude doesn't match WGS84 decimal format" | ||
| And I press the button "Confirm" | ||
| And I wait "1" seconds | ||
| Then I should not see "Please fix the following input errors:" | ||
|
|
||
| Scenario: display 1 warning message when create with wrong latitude below -90 | ||
| Given I am on "/adminSample/create" | ||
| And I should see "Create" | ||
| When I fill in the field of "name" "Sample[species_id]" with "87676:Eucalyptus pauciflora" | ||
| And I fill in the field of "name" "Sample[attributesList]" with "latitude=\"-95.1234\"" | ||
| And I press the button "Create" | ||
| And I wait "1" seconds | ||
| Then I should see "Are you sure you want to continue?" | ||
| And I should see "Attribute value for latitude doesn't match WGS84 decimal format" | ||
| And I press the button "Confirm" | ||
| And I wait "1" seconds | ||
| Then I should not see "Please fix the following input errors:" | ||
|
|
||
| Scenario: display 1 warning message when create with wrong longitude beyond 180 | ||
| Given I am on "/adminSample/create" | ||
| And I should see "Create" | ||
| When I fill in the field of "name" "Sample[species_id]" with "87676:Eucalyptus pauciflora" | ||
| And I fill in the field of "name" "Sample[attributesList]" with "longitude=\"200.1234\"" | ||
| And I press the button "Create" | ||
| And I wait "1" seconds | ||
| Then I should see "Are you sure you want to continue?" | ||
| And I should see "Attribute value for longitude doesn't match WGS84 decimal format" | ||
| And I press the button "Confirm" | ||
| And I wait "1" seconds | ||
| Then I should not see "Please fix the following input errors:" | ||
|
|
||
| Scenario: display 1 warning message when create with wrong longitude below -180 | ||
| Given I am on "/adminSample/create" | ||
| And I should see "Create" | ||
| When I fill in the field of "name" "Sample[species_id]" with "87676:Eucalyptus pauciflora" | ||
| And I fill in the field of "name" "Sample[attributesList]" with "longitude=\"-200.1234\"" | ||
| And I press the button "Create" | ||
| And I wait "1" seconds | ||
| Then I should see "Attribute value for longitude doesn't match WGS84 decimal format" | ||
| And I should see "Are you sure you want to continue?" | ||
| And I press the button "Confirm" | ||
| And I wait "1" seconds | ||
| Then I should not see "Please fix the following input errors:" | ||
|
|
||
| Scenario: display 1 warning message when create with incorrect formatting | ||
| Given I am on "/adminSample/create" | ||
| And I should see "Create" | ||
| When I fill in the field of "name" "Sample[species_id]" with "87676:Eucalyptus pauciflora" | ||
| And I fill in the field of "name" "Sample[attributesList]" with "longitude=\"123.456.789\"" | ||
| And I press the button "Create" | ||
| And I wait "1" seconds | ||
| Then I should see "Are you sure you want to continue?" | ||
| And I should see "Attribute value for longitude doesn't match WGS84 decimal format" | ||
| And I press the button "Confirm" | ||
| And I wait "1" seconds | ||
| Then I should not see "Please fix the following input errors:" | ||
|
|
||
| Scenario: display 1 extra message when create with extra symbol | ||
| Given I am on "/adminSample/create" | ||
| And I should see "Create" | ||
| When I fill in the field of "name" "Sample[species_id]" with "87676:Eucalyptus pauciflora" | ||
| And I fill in the field of "name" "Sample[attributesList]" with "longitude=\"123.4567°\"" | ||
| And I press the button "Create" | ||
| And I wait "1" seconds | ||
| Then I should see "Are you sure you want to continue?" | ||
| And I should see "Attribute value for longitude doesn't match WGS84 decimal format" | ||
| And I press the button "Confirm" | ||
| And I wait "1" seconds | ||
| Then I should not see "Please fix the following input errors:" | ||
|
|
||
| Scenario: display 1 warning message when create with region for lat_lon | ||
| Given I am on "/adminSample/create" | ||
| And I should see "Create" | ||
| When I fill in the field of "name" "Sample[species_id]" with "87676:Eucalyptus pauciflora" | ||
| And I fill in the field of "name" "Sample[attributesList]" with "latitude=something" | ||
| And I press the button "Create" | ||
| And I wait "1" seconds | ||
| Then I should see "Are you sure you want to continue?" | ||
| And I should see "Attribute value for latitude doesn't match WGS84 decimal format. For geographic location (country, sea, region) use another attribute name" | ||
| And I press the button "Confirm" | ||
| And I wait "1" seconds | ||
| Then I should not see "Please fix the following input errors:" | ||
|
|
There was a problem hiding this comment.
suggestion: If these tests are passing, should add @ok tag to them which would be useful when using the acceptance_runner to execute the tests.
kencho51
requested changes
Nov 22, 2024
There was a problem hiding this comment.
issue: According to #2022 (comment), attribute id 269 has been deprecated, latitude (391) and longitude (392) should be used, and below is the query return from the production database:
gigadb=> SELECT attribute_id, value, count(*) FROM sample_attribute WHERE attribute_id =391 GROUP BY attribute_id, value order by count desc;
attribute_id | value | count
--------------+-------------------------------------------------------------------------+-------
391 | 43.25978 | 24194
391 | 39.131595 | 2787
391 | 34.2716 | 2446
391 | 27.12 | 1943
391 | 29.98 | 1816
391 | 21.61 | 1591
391 | 24.53 | 1588
391 | not provided | 909
391 | 34.1273 | 841
391 | -20.7644 | 555
391 | 23.47 | 551
391 | 39.9 | 514
391 | 27.167 | 506
391 | 40.6098 | 436
391 | 24.533 | 419
391 | 21.612 | 418
391 | 22.27 | 406
391 | 23.125 | 405
391 | 48.11 | 401
391 | 41.86 | 400
391 | 42.05 | 356
391 | 22.8 | 355
391 | 60.2264 | 241
391 | 37.07412 | 216
391 | 21.4336 | 215
391 | 55.68 | 202
gigadb=> SELECT attribute_id, value, count(*) FROM sample_attribute WHERE attribute_id = 392 GROUP BY attribute_id, value order by count desc;
attribute_id | value | count
--------------+-----------------------+-------
392 | -79.91763 | 24194
392 | -96.619524 | 2787
392 | 108.083 | 2446
392 | 100.17 | 1943
392 | 102.98 | 1816
392 | 101.57 | 1594
392 | 101.02 | 1587
392 | not provided | 909
392 | 150.7387 | 841
392 | -42.8683 | 555
392 | 116.77 | 551
392 | 100.167 | 506
392 | 114.9434 | 436
392 | 101.016 | 419
392 | 101.574 | 418
392 | 113.287 | 405
392 | 2.78 | 400
392 | 1.8 | 400
392 | -3.29 | 400
392 | 113.46 | 385
392 | 3.2 | 356
392 | 108.37 | 355
392 | 25.0164 | 241
392 | -93.87889 | 216
392 | -157.7883 | 215
392 | 12.57 | 202
gigadb=>
igadb=> SELECT attribute_id, value, count(*) FROM sample_attribute WHERE attribute_id = 270 GROUP BY attribute_id, value order by count desc;
attribute_id | value | count
--------------+--------------------------------------------------------------------------------------------------------------------------------------+-------
270 | Guangzhou, China | 2906
270 | Kansas State University, Ashland Bottoms research farm, USA | 2787
270 | China, Xianyang city, Shanxi province | 2446
270 | China:Yunnan:Lijiang | 1943
270 | China:Sichuan, Ya'an | 1816
270 | China:Yunnan:Nabanhe | 1591
270 | China:Yunnan:Ailaoshan | 1193
270 | Spain | 1147
270 | not provided | 1036
270 | China | 999
270 | France | 942
270 | Australia: NSW | 911
270 | China:Shenzhen | 903
270 | Italy | 748
270 | Brazil | 584
270 | China National GeneBank, BGI-Shenzhen, Shenzhen, China | 543
270 | Germany | 513
270 | USA:St. Paul | 508
270 | China:Yunnan:Yulong Nakhi Autonomous County:Lijiang Forest | 507
270 | Spain: Cala Montgo1, Spain | 485
270 | Switzerland | 461
270 | China, Zhangjiakou city, Hebei province | 436
270 | China:Yunnan:Jingdong Yi Autonomous County:Ailaoshan Forest | 419
270 | China:Mengla County:Mengla:Nanbanhe Forest | 419
270 | China: Guangdong General Hospital | 405
270 | China:Yunnan:Ailaoshan | 394
suggestion: Is it better to make the adminSample form to display attribute results from id 371, 372 and 270 ?
alli83
force-pushed
the
fix-2022-sample-attribute-lat-lon-regex-and-migration
branch
from
f25ddc3 to
653665f
Compare
|
linked to #1995 |
alli83
force-pushed
the
fix-2022-sample-attribute-lat-lon-regex-and-migration
branch
2 times, most recently
from
10213b3 to
dfb7eff
Compare
alli83
force-pushed
the
fix-2022-sample-attribute-lat-lon-regex-and-migration
branch
from
dfb7eff to
b6b41d1
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull request for issue: #831
This is a pull request for the following functionalities:
check attribute modal (latitude and longitude) + tests
How to test?
connect as admin
click samples
edit or create a ne sample
in attribute list field add longitude=200.1234 e.g
click save
you should see a popup alert with "Attribute value for longitude doesn't match WGS84 decimal format"
you can confirm it if you want to override the alert and save
How have functionalities been implemented?
add method in adminSampleController
Any issues with implementation?
Any changes to automated tests?
test added
Any changes to documentation?
Any technical debt repayment?
Any improvements to CI/CD pipeline?