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

🤓 Record dependencies and result names in the definition after flow validation #551

Merged
merged 6 commits into from
Mar 1, 2019

Conversation

rowanseymour
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Mar 1, 2019

Codecov Report

Merging #551 into master will decrease coverage by 0.09%.
The diff coverage is 62.83%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #551     +/-   ##
========================================
- Coverage   75.89%   75.8%   -0.1%     
========================================
  Files         175     175             
  Lines        9144    9154     +10     
========================================
- Hits         6940    6939      -1     
+ Misses       1805    1803      -2     
- Partials      399     412     +13
Impacted Files Coverage Δ
flows/actions/add_contact_urn.go 83.87% <ø> (-0.98%) ⬇️
flows/actions/play_audio.go 89.65% <ø> (-0.67%) ⬇️
flows/actions/set_contact_timezone.go 93.1% <ø> (-0.45%) ⬇️
flows/actions/call_resthook.go 82.6% <ø> (-0.73%) ⬇️
flows/actions/add_input_labels.go 90.47% <ø> (-0.83%) ⬇️
flows/actions/enter_flow.go 90.47% <ø> (+12.21%) ⬆️
flows/templates.go 46.66% <ø> (+16.66%) ⬆️
flows/actions/set_run_result.go 92.3% <ø> (-0.55%) ⬇️
flows/actions/say_msg.go 84.37% <ø> (-0.92%) ⬇️
flows/actions/set_contact_field.go 86.66% <ø> (-1.22%) ⬇️
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb33d2b...93b4d89. Read the comment docs.

// Validate validates our action is valid and has all the assets it needs
func (a *TransferAirtimeAction) Validate(assets flows.SessionAssets, context *flows.ValidationContext) error {
// Validate validates our action is valid
func (a *TransferAirtimeAction) Validate() error {
Copy link
Member Author

Choose a reason for hiding this comment

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

actions now only do structural validation on themselves - they're not responsible for determining if their dependencies exist - we do that centrally

@@ -11,7 +11,7 @@
}
]
},
"validation_error": "no such group with UUID '33382939-babf-4982-9395-8793feb4e7c6'"
"validation_error": "missing dependencies: group[uuid=33382939-babf-4982-9395-8793feb4e7c6,name=Climbers]"
Copy link
Member Author

Choose a reason for hiding this comment

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

this should never end up being a user facing error.. assuming we track dependencies correctly

a, err := sa.Fields().Get(ref.Key)

if err != nil {
// TODO for now if a field reference came from an expression (i.e. no name), we don't blow up if it's missing
Copy link
Member Author

Choose a reason for hiding this comment

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

chatted with @nicpottier and we can look at addressing this in the old world but there are just too many flows out there that likely to have expressions referencing fields that don't exist

f.valid = true
f.dependencies = deps
f.resultNames = f.ExtractResultNames()
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is threadsafe... it's possible that multiple threads validate this flow object at the same time, but it shouldn't matter

if r != nil && !r.Variable() && !dependenciesSeen[key] {
dependencies = append(dependencies, r)
dependenciesSeen[key] = true
if !utils.IsNil(r) && !r.Variable() {
Copy link
Member Author

Choose a reason for hiding this comment

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

See #534


// finally check that all the groups exist
return a.validateGroups(assets, a.Groups)
// Validate validates our action is valid
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe no-op Validate should be part of BaseAction?

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

Successfully merging this pull request may close these issues.

2 participants