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

Fix bug of setting skipped class to step links #182

Closed
wants to merge 2 commits into from

Conversation

simohammedhttp
Copy link

No description provided.

Normally craue_formflow_skipped_step class must be provided to steps after the condition in pevious steps is fullfilled.
@craue
Copy link
Owner

craue commented Jun 1, 2015

Could you explain the bug in more detail, maybe with an example? I don't see what's wrong with the current implementation. Is it an edge case?

@simohammedhttp
Copy link
Author

I noticed that when I load the form for the first time all the skippable steps are set with craue_formflow_skipped_step which is weird.
Let's assume we have a flow of two steps: Step1 and Step2, Step1 contains usedProducts property (collection of checkboxes), Step2 must be skipped if usedProducts != 'ProductX' (the product being studied in survey).
When I load the survey, Step2 is marked skipped, even I haven't responded to Step1 yet.

@craue
Copy link
Owner

craue commented Jun 1, 2015

The current implementation allows marking succeeding steps as skipped, which is correct. So make sure that your skip function always evaluates to the expected result. Maybe something like this for step 2:

'skip' => function($estimatedCurrentStepNumber, FormFlowInterface $flow) {
    return $estimatedCurrentStepNumber > 1 && !$flow->getFormData()->hasUsedProduct('ProductX');
}

@coveralls
Copy link

Coverage Status

Coverage decreased (-19.22%) to 80.18% when pulling 05cdd1e on simohammedhttp:master into ffbf596 on craue:master.

@simohammedhttp
Copy link
Author

Mmm..
It sounds interesting, I'll give it a try.

@craue
Copy link
Owner

craue commented Jun 10, 2015

@simohammedhttp: Is it ok for you to close this PR?

@simohammedhttp
Copy link
Author

Yes, I think the actual implementation is quite good.

2015-06-10 13:40 GMT+01:00 Christian Raue notifications@github.com:

@simohammedhttp https://github.com/simohammedhttp: Is it ok for you to
close this PR?


Reply to this email directly or view it on GitHub
#182 (comment)
.

@craue craue closed this Jun 11, 2015
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.

None yet

3 participants