Line of Sight GP tool needs checks for valid input parameters

kgonzago commented 6 years ago

Task to be done 16 May 2018

[x] Verify tool does not work properly in ArcMap and Pro
[x] Add validation in the execution step
[x] Verify automated tests for no observers and targets
[x] Verify Manually
[x] Verify in Military Tools Build https://github.com/Esri/military-tools-geoprocessing-toolbox/pull/313
Line of Sight GP tool needs to have checks in place to determine if input parameters are valid for observers and targets.

Expected Behavior

After testing in Pro, the tool can be executed without supplying either an observer or target input parameter. The tool will eventually fail and occasionally will crash ArcGIS Pro entirely.

Current Behavior

Currently, the tool accepts zero to many observers and targets. The tool should accept one to many observers and targets.

Possible Solution

A valid check for observer and target input parameters need to implemented. This will be part of the tool validation process.

Steps to Reproduce (for bugs)

Open Pro
Add a DEM
Launch Line of Sight tool
Add Observers
Do not add Targets
Execute tool

Context

This issue was found when testing the Line of Sight tool on another issue.

Your Environment

Version used: Pro 2.1
Environment name and version (e.g. Chrome 39, node.js 5.4): N/A
Operating System and version (desktop or mobile): Win10
Link to your project:

kgonzago commented 6 years ago

Issue fixed in this PR

kgonzago commented 6 years ago

Test cases were added and are reflected in this PR

topowright-zz commented 6 years ago

I am looking at the pull request for this issue. https://github.com/Esri/military-tools-geoprocessing-toolbox/pull/313

dfoll commented 6 years ago

tested this on @kgonzago "kg/#302" branch and got the expected results from the fix which is for the tool to run and fail with a very clear error message to "Please provide at least one observer" or "Please provide at least one target" if there is an observer but no target. Cannot test from build
no doc changes, so I'm removing G-QC (Doc)
created checklist test for LLOS tool in Visibility_GP.md

topowright-zz commented 6 years ago

IMPEDED -- Needs to be tested from the build. Waiting on the build to be finished. To test any bug or feature it must be tested from a build that has all the unit test passing.

dfoll commented 6 years ago

picked this up to test this morning. The current version of the tool in the build and on dev is faulty in that it does not have all the parameters listed "Targets" is missing from the dialog. Restored @kgonzago branch "kg/302" and the tool looks ok there. Somewhere along the way a PR got merged that broke this tool. Assigning this for now @csmoore or @kgonzago let me know when one of you has time to look at this and I can pick it up again

UPDATE: fixed in PR #332 this can be tested once there is a build 36 or later

kgonzago commented 6 years ago

@dfoll Parameters were incorrect. Fixed it in this PR

dfoll commented 6 years ago

verified in the .pyt with MT build 40

Esri / military-tools-geoprocessing-toolbox