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

MatchValidator broken due to CompoundWidget state #110

Merged
merged 1 commit into from Feb 17, 2014
Merged

MatchValidator broken due to CompoundWidget state #110

merged 1 commit into from Feb 17, 2014

Conversation

amol-
Copy link
Contributor

@amol- amol- commented Feb 15, 2014

Provides a test unit to expose that MatchValidator stopped working due to change in CompoundWidget now correctly passing a state when validating (this is required for compatibility on i18n with FormEncode). The proposed fix provides the values of the CompoundWidget inside the state when it gets validated. The values are available inside full_dict for coherence with FormEncode and states are copied before updating them to provide a stack of states instead of replacing the original one.

Provides a test unit to expose that MatchValidator stopped working due to change in CompoundWidget now correctly passing a state when validating (this is required for compatibility on i18n with FormEncode). The proposed fix provides the values of the CompoundWidget inside the state when it gets validated. The values are available inside full_dict for coherence with FormEncode and states are copied before updating them to provide a stack of states instead of replacing the original one.
@amol-
Copy link
Contributor Author

amol- commented Feb 17, 2014

Can I merge this myself? is everyone fine with the change?

@ralphbean
Copy link
Contributor

👍 :)

amol- added a commit that referenced this pull request Feb 17, 2014
MatchValidator broken due to CompoundWidget state
@amol- amol- merged commit 92324f0 into toscawidgets:develop Feb 17, 2014
@amol- amol- deleted the master branch February 17, 2014 19:54
@lukash
Copy link

lukash commented May 24, 2014

Would it be possible to make a release with this fix? The MatchValidator is broken without it.

moschlar added a commit to toscawidgets/tw2.forms that referenced this pull request Jul 25, 2014
@moschlar
Copy link
Member

Hey @amol- , could you have a look at the (newly added) broken test case that fails because of this change?

  File "/home/moschlar/.virtualenvs/tw2/lib/python2.7/site-packages/tw2/core/widgets.py", line 523, in validate
    return ins._validate(value, state)
  File "/home/moschlar/.virtualenvs/tw2/lib/python2.7/site-packages/tw2/forms/widgets.py", line 744, in _validate
    StripBlanks().to_python(value, state), state
  File "/home/moschlar/.virtualenvs/tw2/lib/python2.7/site-packages/tw2/core/validation.py", line 106, in wrapper
    d = fn(self, *args, **kw)
  File "/home/moschlar/.virtualenvs/tw2/lib/python2.7/site-packages/tw2/core/widgets.py", line 871, in _validate
    data.append(self.children[i]._validate(v, data))
  File "/home/moschlar/.virtualenvs/tw2/lib/python2.7/site-packages/tw2/core/validation.py", line 106, in wrapper
    d = fn(self, *args, **kw)
  File "/home/moschlar/.virtualenvs/tw2/lib/python2.7/site-packages/tw2/core/widgets.py", line 669, in _validate
    state = util.clone_object(state, full_dict=value, validated_values=data)
  File "/home/moschlar/.virtualenvs/tw2/lib/python2.7/site-packages/tw2/core/util.py", line 139, in clone_object
    setattr(obj, k, v)
AttributeError: 'list' object has no attribute 'validated_values'

Encountered this while trying things for toscawidgets/tw2.forms#38

@amol-
Copy link
Contributor Author

amol- commented Jul 25, 2014

16edf65 should fix issue with wrong state. But the test is still failing as it expects a different result.

Is it meant that the test expects None as the return value for the validation call?

As we are validating {'grid:0:field1': 'something'} it should actually at least get back a value for field1 right?

@moschlar
Copy link
Member

Ah, well, no...
I just didn't come that far with the test... :D

Thanks for the fix! ;)

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

4 participants