required=True breaks design symmetry with tw2.core

[ralphbean]

@paj28 and I have been corresponding about this over the mailing list. Creating a ticket here so that it gets fixed at some point.

---

I'm trying to understand what has been done around "required" and
RelatedItemValidator and RelatedOneToOneValidator. For the rest of
Toscawidgets, widgets don't have a required parameter, instead you set
"validator = twc.Required" - or if you want to further customise the
validator "validator = twc.MyValidator(required=True)". I would prefer to
keep things consistent.

This is very recent and an oversight on my part for not seeing the
break in design symmetry. I say go right ahead with the refactor at
your leisure.

I had a go at this, and I'm struggling to do it and keep the tests passing!
I will try some more when I get some time, or if you fancy having a go,
that'd be fab.

---

I think this was introduced in Pull Request #6.

[LeResKP]

Hello,

It seems I put this problem and I don't understand it. The 'required' is set according to the DB. If I can help to fix this issue, tell me.

Aurélien

[paj28]

Hi,

Say you are using DbSingleSelectField directly - and not using
AutoTableForm. With a normal SingleSelectField you would set required like
this:

SingleSelectField(validator=twc.Required)

I think we should carry that model into DbSingleSelectField. In fact, I
think it works now, but I don't like the redundancy of having two ways to
say it. Also, I don't really get RelatedItemValidator and
RelatedOneToOneValidator - I have a feeling they're not really required and
would like to remove them to keep things as simple as possible.

Paul

On Tue, Jul 31, 2012 at 11:22 PM, LeResKP <
[email protected]

wrote:

Hello,

It seems I put this problem and I don't understand it. The 'required' is
set according to the DB. If I can help to fix this issue, tell me.

Aurélien

---

Reply to this email directly or view it on GitHub:
#10 (comment)

[paj28]

I've just realised this has introduced a more serious bug.

All the DbSelect* fields become required by default. This is because they inhertit from tw2.forms.FormField, which defines a "required" property.

This does need fixing quite urgently - it's broken an app of mine here.

[ralphbean]

Sorry I'm late at chiming into this.. I had some family issue to attend to for a few weeks.

@paj28, do you have a solution in your repos? I could start in on a fix now, but I don't want to duplicate your work (or worse) come up with a third competing, unmergable strategy.

[paj28]

Hi,

No worries Ralph, hope everything is ok now.

I have hacked at it a little, but nothing ready to commit. Basically the
plan was to remove "required" complete from widgets.py - instead users say
"validator = twc.Required" factory.py will need to be updated to follow
this pattern. I did attempt this and it broke a lot of unit tests, which I
stated to look at, but it's a bit of a daunting job.

Paul

On Fri, Aug 17, 2012 at 3:10 PM, Ralph Bean [email protected]:

Sorry I'm late at chiming into this.. I had some family issue to attend to
for a few weeks.

@paj28 https://github.com/paj28, do you have a solution in your repos?
I could start in on a fix now, but I don't want to duplicate your work (or
worse) come up with a third competing, unmergable strategy.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/10#issuecomment-7822884.

[ralphbean]

I think some of the tests were modified in pull request #6. You can probably modify/remove the tests that were modified/added there. That might make the work a little easier.

[LeResKP]

I can make the updates if you want, my vacations are finished.... But one thing was not clear to me: If the field is not nullable in the DB and the user doesn't pass a validator, you are ok that we set the validator as twc.Required?

Aurélien

[paj28]

Hi,

Yes, that's exactly what it should do. tw2.core has specific support for
twc.Required - it clones the base class validator, but adds required=True.

Paul

On Mon, Aug 20, 2012 at 1:14 PM, LeResKP [email protected] wrote:

I can make the updates if you want, my vacations are finished.... But one
thing was not clear to me: If the field is not nullable in the DB and the
user doesn't pass a validator, you are ok that we set the validator as
twc.Required?

Aurélien

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/10#issuecomment-7867341.

[LeResKP]

Hello,

I had removed the required parameters:
LeResKP@5c88312

For the one to one relation, it's hard to remove it since it uses AutoEditFieldSet which inherit from AutoContainer. I don't know why but the validator twc.Required I pass is lost. It seems it's updated on the fly in tw2.core.widget with the post_define functions...

I see you want to remove the RelatedOneToOneValidator and RelatedItemValidator but I'm not sure it's a good idea since we need to validate the following cases:

Example for RelatedOneToOneValidator:

class Account(Base):
tablename = 'user'
id = sa.Column(sa.Integer, primary_key=True)
name = sa.Column(sa.String(50), nullable=True)
pwd = sa.Column(sa.String(50), nullable=True)

class User(Base):
tablename = 'Account'
id = sa.Column(sa.Integer, primary_key=True)
name = sa.Column(sa.String(50))
account_id = sa.Column(sa.Integer, sa.ForeignKey('user.id'), nullable=False)
account = sa.orm.relation(User, backref=sa.orm.backref('user', uselist=False))

When we add a User we need an Account. Since nothing was required in Account, we need to validate that we have at least one value to make sure we will create the Account object. If it's not the case we will have an IntegrityError since account_id will be None.

For RelatedItemValidator:
if we want to have a DbCheckBoxList 'required' (with at least one value checked), we need to validate it.

Aurélien

[paj28]

Hi,

That looks good to me, and it solves the problem I had of all the fields
becoming required.
I've pushed the changes to https://github.com/paj28/tw2.sqla

Example for RelatedOneToOneValidator:
When we add a User we need an Account. Since nothing was required in
Account, we need to validate that we have at least one value to make sure
we will create the Account object. If it's not the case we will have an
IntegrityError since account_id will be None.

I'm not sure I completely understand this, but if you need it, fair enough.

For RelatedItemValidator:
if we want to have a DbCheckBoxList 'required' (with at least one value
checked), we need to validate it.

This should be done by ListLengthValidator in tw2.core. Can you remove
RelatedItemValidator?

Paul

[LeResKP]

Hello,

That looks good to me, and it solves the problem I had of all the fields becoming required.
I've pushed the changes to https://github.com/paj28/tw2.sqla

Cool. Do you want I create a pull request to toscawidgets?

This should be done by ListLengthValidator in tw2.core. Can you remove
RelatedItemValidator?

I just tried to remove RelatedItemValidator but now I remember why I created this validator.

In this case:
validator = ListLengthValidator => Validate only the length of the list
item_validator = RelatedValidator => Validate the values are correct

But If I pass an incorrect value (not in the DB) the ListLengthValidator doesn't raise an exception since we have a value, and RelatedValidator will remove this incorrect value without raising an exception (twc.safe_validate is used for the item_validator).

So I need a validator which only count the validated values in case the user has tampered the form...

Aurélien

[ralphbean]

Any resolution on this bug? @paj28, I see you've merged @LeResKP's changes into your repo.

Are the commits in a state where they can be merged back into the toscawidgets mainline repo so we can close this issue?

[paj28]

Hi,

The change I merged is good to go on the mainline repo.

There is one further change I'd like to make, but it's just a minor tidy
up, I'll do it as and when.

Paul

On Wed, Sep 5, 2012 at 4:59 PM, Ralph Bean [email protected] wrote:

Any resolution on this bug? @paj28 https://github.com/paj28, I see
you've merged @LeResKP https://github.com/LeResKP's changes into your
repo.

Are the commits in a state where they can be merged back into the
toscawidgets mainline repo so we can close this issue?

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/10#issuecomment-8303912.

[ralphbean]

Hum, I'm getting an infinite recursion error when I try to run the tests on the develop branch of your repo.

Note -> if you issue a pull request against the main toscawidgets repo, the Travis Bot should automatically run the tests and report on the pull request ticket. :)

[volkmuth]

From a bisect it looks like the infinite recursion error is caused by commit b8877bb (when working with toscawidgets/tw2.core@3288a59029 and toscawidgets/tw2.forms@0578d04e9)

[volkmuth]

@ralphbean, I think the infinite recursion error is caused because reserved_names in @toscawidgets/tw2core3288a59 tw2.core.widgets does not include 'partial_parent'. See paj28/tw2.core@7f30e5fa where it looks like partial_parent was introduced.