Skip to content

[7.0] [add] fields validator - #84

Merged
gurneyalex merged 15 commits into
OCA:7.0from
eLBati:7_add_fields_validator
Aug 9, 2016
Merged

gurneyalex merged 15 commits into
OCA:7.0from
eLBati:7_add_fields_validator

Conversation

@eLBati

@eLBati eLBati commented Nov 17, 2014

Copy link
Copy Markdown
Member

This module allows to set a regular expresion as field validator.
When the regular expresion is set, write and create operations on the involved
field are blocked, if the regular expression is not satisfied.
See demo and test data for an example with partner email.

@pedrobaeza

Copy link
Copy Markdown
Member

Hi, @eLBati, thanks for the module. I think a better name for it is base_field_validator

Entering in the functionality, I feel that this kind of things must be done client-side (in JS widgets), not server-side. In this direction, there is this PR OCA/web#34 and collides in functionality with this one. We should decide in which direction go.

@eLBati

eLBati commented Nov 18, 2014

Copy link
Copy Markdown
Member Author

Hi @pedrobaeza thanks for pointing it out (I don't follow https://github.com/OCA/web enough 😔)
It is fine for me to add web_fields_masks only.
We are going to test it.

@eLBati eLBati closed this Nov 18, 2014
@eLBati

eLBati commented Nov 20, 2014

Copy link
Copy Markdown
Member Author

Not sure if web_fields_masks supports regex (OCA/web#34 (comment)). It seems it does not

@eLBati eLBati reopened this Dec 15, 2014
@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+2.82%) when pulling 68461c9 on eLBati:7_add_fields_validator into e6f0c68 on OCA:7.0.

@eLBati

eLBati commented Mar 9, 2015

Copy link
Copy Markdown
Member Author

Just to notify that this needs review (can't edit labels)

@pedrobaeza

Copy link
Copy Markdown
Member

Don't you think it's better to add (or check) regex support on the other PR. This is to follow with my thought about having this kind of checks client-side.

@lmignon

lmignon commented Mar 9, 2015

Copy link
Copy Markdown
Contributor

@pedrobaeza IMO, the client side would stay without business logic... (and field validation is part of the business logic). If I understand well the PR OCA/web#34, the validation doesn't prevent to save the value. In this PR, the validation is enforced even on the server side. I've the feeling that this PR covers another UC. What do you think?

@pedrobaeza

Copy link
Copy Markdown
Member

I think this is not business logic, but basic validations. Check for example HTML forms: this kind of validations (mandatory fields, numeric values, etc) are made always client side.

@nhomar

nhomar commented Mar 9, 2015

Copy link
Copy Markdown
Member

I think exactly as @lmignon It is a good feature, if you go throught a customized clients over web-servides the OCA/web#34 will be jumped.

+1 to have it in both sides (we need both).

Client side:

To avoid the POST that consume time and make a uncommit of the changes.

Server side:

To be consistent thoruhgt web-services.

On my side a total +1 "Conceptually Speaking" to this PR.

@lmignon

lmignon commented Mar 10, 2015

Copy link
Copy Markdown
Contributor

For sure the validation on the client will leverage the User experience and avoid unnecessary calls to the server. But it's the responsibility of the server to enforce the data consistency whatever they are modified (by a request from the browser or from web-services or even internal calls). Conceptually the concept of validator sounds good. One step further would be to leverage the fields_view_get from BaseModel to add the validator definition in the field description send to the JS client and implement the validation also on the client side.... (I've a dream...)

@pedrobaeza

Copy link
Copy Markdown
Member

Yeah, we'll have to put it as wishlist.

@eLBati, please consider following remarks then and I'll approve it:

  • Change name to base_field_validator
  • Add a know issue / roadmap section in description about the lack of client side check.

@eLBati
eLBati force-pushed the 7_add_fields_validator branch from 68461c9 to eac591c Compare March 15, 2015 09:53
@eLBati

eLBati commented Mar 15, 2015

Copy link
Copy Markdown
Member Author

Thanks for the reviews
@pedrobaeza done

@pedrobaeza

Copy link
Copy Markdown
Member

👍 Thanks

@max3903 max3903 modified the milestone: 7.0 May 26, 2015
@yvaucher

yvaucher commented Jun 2, 2015

Copy link
Copy Markdown
Member

commits from hurrinico are not well defined, it is certainly missing email in author

Can you amend them?

@eLBati
eLBati force-pushed the 7_add_fields_validator branch from b82707e to 5af4e1a Compare June 4, 2015 10:41
@eLBati

eLBati commented Jun 4, 2015

Copy link
Copy Markdown
Member Author

On 2 June 2015 at 17:33, Yannick Vaucher notifications@github.com wrote:

commits from hurrinico are not well defined, it is certainly missing email
in author

Can you amend them?

Amended!

Comment thread base_field_validator/__openerp__.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

s/expresion/expression

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done, thanks

@eLBati
eLBati force-pushed the 7_add_fields_validator branch from 5af4e1a to a609400 Compare June 4, 2015 13:12
@yvaucher

yvaucher commented Jun 9, 2015

Copy link
Copy Markdown
Member

👍

@eLBati

eLBati commented Jun 29, 2015

Copy link
Copy Markdown
Member Author

I think this can be merged

Comment thread base_field_validator/ir_model.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you should return whatever old_write returns

@eLBati
eLBati force-pushed the 7_add_fields_validator branch from 93e8736 to fa3f23b Compare September 2, 2015 10:31
@eLBati

eLBati commented Sep 2, 2015

Copy link
Copy Markdown
Member Author

@hbrunn thanks, I made the proposed changes.
Now I try to find out why travis failed

@eLBati
eLBati force-pushed the 7_add_fields_validator branch from fa3f23b to 68d6179 Compare September 2, 2015 13:56
@eLBati

eLBati commented Sep 2, 2015

Copy link
Copy Markdown
Member Author

Ok, my bad. Now travis is green 💚

@eLBati

eLBati commented Dec 15, 2015

Copy link
Copy Markdown
Member Author

@OCA/tools-maintainers please set this 'needs review'

@StefanRijnhart

Copy link
Copy Markdown
Member

As on the 8.0 PR, better not to change the signature of _register_hook

@guewen

guewen commented Jun 6, 2016

Copy link
Copy Markdown
Member

Can we merge this PR?

@gurneyalex
gurneyalex merged commit 5ecbf79 into OCA:7.0 Aug 9, 2016
@gurneyalex

Copy link
Copy Markdown
Member

👍

nhomar pushed a commit that referenced this pull request May 5, 2017
[ADD] company_country: module to install country from .travis.yml
Garamotte pushed a commit to subteno-it/server-tools that referenced this pull request Jul 12, 2017
* FIX format parameter

To be able to print files in raw format it's needed to set this parameter.

Previous code was not able to print raw files, but it didn't get error because 'format' used on line 107 is assumed as a format funtion, because format parameter was not on this scope.

* FIX pep8
modoolar-bot pushed a commit to modoolar/oca-server-tools that referenced this pull request Jan 14, 2022
Signed-off-by simahawk
SiesslPhillip pushed a commit to grueneerde/OCA-server-tools that referenced this pull request Nov 20, 2024
Syncing from upstream OCA/server-tools (10.0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.