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

[10.0][ADD] website_form_builder: Exactly what the title says #402

Merged
merged 12 commits into from Feb 2, 2018

Conversation

Projects
None yet
5 participants
@Yajo
Copy link
Member

commented Dec 15, 2017

This is WIP until tours pass, but you can start playing with it!

@Yajo Yajo self-assigned this Dec 15, 2017

@Yajo Yajo added this to the 10.0 milestone Dec 15, 2017

@Yajo Yajo force-pushed the Tecnativa:10.0-website_form_builder branch from 178c7a3 to b913b12 Dec 15, 2017

@pedrobaeza pedrobaeza changed the title [ADD] website_form_builder: Exactly what the title says [10.0][ADD] website_form_builder: Exactly what the title says Dec 15, 2017

@pedrobaeza

This comment has been minimized.

Copy link
Member

commented Dec 15, 2017

@lasley I think you will love this

@lasley

This comment has been minimized.

Copy link
Member

commented Dec 15, 2017

Oh snap!

sense in this module's context, or a correct implementation would be adding
not much value while adding lots of complexity:

* ``id``

This comment has been minimized.

Copy link
@pedrobaeza

pedrobaeza Dec 15, 2017

Member

What about create_date, write_date, create_uid, write_uid? They should be forbidden also I think.

installation to get a better UX when a user has already sent a form and
cannot resend it.

* To edit any ``<label>`` text, you need to click twice. Review the problem

This comment has been minimized.

Copy link
@pedrobaeza

pedrobaeza Dec 15, 2017

Member

Only in Firefox?

This comment has been minimized.

Copy link
@Yajo

Yajo Dec 18, 2017

Author Member

Chrome worked without patch out of the box. Besides, this bug affects EE website_form_editor, just try in the enterprise runbot.

@lasley

lasley approved these changes Dec 15, 2017

Copy link
Member

left a comment

Freaking awesome @Yajo. Haven't played with it functionally yet, but excellent code. A few minor things inline

fields. When you create a new website form, all its model fields are
automatically whitelisted for the sake of improving the UX. If you want to
have higher control, come back here after creating the form and blacklist
any fields you want.

This comment has been minimized.

Copy link
@lasley

lasley Dec 15, 2017

Member

Hmmm this is opposite of the operation of the existing website_form, which is a whitelist. More thoughts probably after reading the code, this is mainly a reminder for me to update with more thoughts. If I don't, remind me to update with more thoughts!

This comment has been minimized.

Copy link
@pedrobaeza

pedrobaeza Dec 15, 2017

Member

I prefer this approach for not having to manually add each of the fields we wish to add or supply glue modules for each possible model.

This comment has been minimized.

Copy link
@lasley

lasley Dec 15, 2017

Member

(moved inline)

Hmmm but modules that are naive of forms, which inherit a model that is be exposed by a form, typically expect that the data is coming from an authenticated source. An example is prescription or doctor verification. This data can now be submitted by users that are not authenticated.

This comment has been minimized.

Copy link
@pedrobaeza

pedrobaeza Dec 15, 2017

Member

Models are opt-in, fields are opt-out

This comment has been minimized.

Copy link
@lasley

lasley Dec 15, 2017

Member

In the example I was mentioning, the validation field is on the partner. I see a lot of potential uses for opting the partner in, which would opt in those fields too.

I'm actually kind of confused though. This black list is circumventing the pre-existing whitelist of website_form? Why is the whitelist also being used in the demo data?

This comment has been minimized.

Copy link
@Yajo

Yajo Dec 18, 2017

Author Member

Well, the point here is that, in an awesomely rare exception, website_form is completely undocumented (🤣). I tried to reuse the existing engine as much as possible, and it actually makes sense. Let me explain what it does:

sense in this module's context, or a correct implementation would be adding
not much value while adding lots of complexity:

* ``id``

This comment has been minimized.

Copy link
@lasley

lasley Dec 15, 2017

Member

IMO we should prevent all the magic fields

This comment has been minimized.

Copy link
@Yajo

Yajo Dec 18, 2017

Author Member

That happens automatically, I was just trying to avoid having to fill the docs gap from website_form, but I think I'll have to do it after all 😆

"/website_form_builder/static/src/xml/snippets.xml"
);

var _fields_asked = {},

This comment has been minimized.

Copy link
@lasley

lasley Dec 15, 2017

Member

IMO we should use JS naming conventions & camelCase the vars/methods

This comment has been minimized.

Copy link
@Yajo

Yajo Dec 18, 2017

Author Member

I know those common JS conventions, but in Odoo it seems they decided to match the python ones. Just give a glance at https://www.odoo.com/documentation/11.0/reference/javascript.html and you'll see. So, I prefer to stick with those.

*/
add_model_field: function (info) {
var relational_data = [],
template = _.str.sprintf(

This comment has been minimized.

Copy link
@lasley

lasley Dec 15, 2017

Member

TIL _.str

var domain = [], context = base.get_context();
// Domain might contain un-evaluable literals
try {
domain = new data.CompoundDomain(field.domain || []).eval()

This comment has been minimized.

Copy link
@lasley

lasley Dec 15, 2017

Member

Missing ; * 3 other instances below

this.field_html = $(field).html();
options = $.extend({}, {
title: _t("Set field's default value"),
size: "small",

This comment has been minimized.

Copy link
@lasley

lasley Dec 15, 2017

Member

trailing comma no bueno in JS

This comment has been minimized.

Copy link
@Yajo
#. Search for the model you want to manage website form access for.
#. When you find it, it will have a *Website Forms* section where you can:

* Allow the model to get forms, by checking *Allowed to use in forms*.

This comment has been minimized.

Copy link
@pedrobaeza

pedrobaeza Dec 15, 2017

Member

We need to provide a built-in list of allowed models without depending on the corresponding modules (crm.lead if present, hr.recruitment if present...). We can use a similar approach to the one used in website_multi_theme.

This comment has been minimized.

Copy link
@Yajo

Yajo Dec 18, 2017

Author Member

As I just mentioned above in #402 (comment), addons that depend on website_form already provide those, so not needed IMO.

@Yajo

This comment has been minimized.

Copy link
Member Author

commented Dec 18, 2017

I think all your concerns are solved now, let me focus on the tour. However functional reviews can start already 😊

@Yajo Yajo added the needs review label Dec 18, 2017

@Yajo Yajo removed the work in progress label Dec 18, 2017

@Yajo

This comment has been minimized.

Copy link
Member Author

commented Dec 18, 2017

Ok, it went ✔️ locally, let's see in Travis. The tour is not as exhaustive as I'd wish, but it's a good start IMHO.

@tarteo
Copy link
Member

left a comment

@Yajo Very useful module!

I followed the steps provided by the readme using model project and task but I got stuck when I tried to whitelist a field. The error I got was: "Properties of base fields cannot be altered in this manner! Please modify them through Python code, preferably through a custom addon!".

It also would be nice to be able to edit "Blacklisted in web forms" from the Model form. Now it's only available through the Field form (Settings -> Technical -> Database Structure -> Fields).

@Yajo

This comment has been minimized.

Copy link
Member Author

commented Jan 10, 2018

AFAIK the setting is available in the model form too:

captura de pantalla de 2018-01-10 10-40-28

I followed the steps provided by the readme using model project and task but I got stuck when I tried to whitelist a field. The error I got was: "Properties of base fields cannot be altered in this manner! Please modify them through Python code, preferably through a custom addon!".

Could you provide the steps please?

@tarteo

This comment has been minimized.

Copy link
Member

commented Jan 10, 2018

@Yajo

  • Activate developer mode
  • Go to Technical -> Database Structure -> Models
  • Check "Allowed to use in forms"
  • Click a field in the Fields tab
  • A modal form opens with the field form
  • "Blacklisted in web forms" is not there
  • Go to Technical -> Database Structure -> Fields
  • Search for Project
  • Click the name field
  • "Blacklisted in web forms" is there
  • Uncheck "Blacklisted in web forms"
  • Click Save
  • A message shows: "Properties of base fields cannot be altered in this manner! Please modify them through Python code, preferably through a custom addon!"
@Yajo

This comment has been minimized.

Copy link
Member Author

commented Jan 11, 2018

It seems you can't alter fields properties through UI unless it's a manually-created model. I'm not sure I want to change that... Should I then just change the docs?

@tarteo

This comment has been minimized.

Copy link
Member

commented Jan 11, 2018

@Yajo haha, that would be funny! So the module will only work for manual created models through the UI. That's np, it's still a very useful module. Maybe you can add a disclaimer to the readme.

@Yajo Yajo force-pushed the Tecnativa:10.0-website_form_builder branch from c993b4e to 15825b7 Jan 16, 2018

@Yajo

This comment has been minimized.

Copy link
Member Author

commented Jan 16, 2018

Docs fixed

@Yajo Yajo requested a review from rafaelbn Jan 16, 2018

@tarteo

tarteo approved these changes Jan 16, 2018

Copy link
Member

left a comment

👍

@Yajo Yajo added work in progress and removed needs review labels Jan 19, 2018

@Yajo

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2018

Setting as WIP again to fix the hidden fields UI

@Yajo Yajo added needs review and removed work in progress labels Jan 19, 2018

@Yajo

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2018

Done. Now, instead of having the previously ugly and unusable UI for hidden data, now the user can simply set the field as hidden and use it as usual. Since hidden fields are expected to have a value, when hiding a field without it, the user will be asked for it automatically.

Please @rafaelbn review

@rafaelbn

This comment has been minimized.

Copy link
Member

commented Jan 22, 2018

Hi @Yajo 👍

Please try the form I've created in http://3315250-402-88cc0c.runbot1.odoo-community.org/#scrollTop=0

Form says "An error has occured, the form has not been sent."

-----------------------------27635325922190
Content-Disposition: form-data; name="name"

Mismo asunto para todos
-----------------------------27635325922190
Content-Disposition: form-data; name="campaign_id"

2
-----------------------------27635325922190
Content-Disposition: form-data; name="partner_name"

Bodeguita
-----------------------------27635325922190
Content-Disposition: form-data; name="email_from"

a@a
-----------------------------27635325922190
Content-Disposition: form-data; name="phone"

+34 123 456 7891
-----------------------------27635325922190
Content-Disposition: form-data; name="mobile"

+34666888444
-----------------------------27635325922190
Content-Disposition: form-data; name="street"

street
-----------------------------27635325922190
Content-Disposition: form-data; name="state_id"

28
-----------------------------27635325922190
Content-Disposition: form-data; name="zip"

28045
-----------------------------27635325922190
Content-Disposition: form-data; name="description"

Notas largas
-----------------------------27635325922190
Content-Disposition: form-data; name="tag_ids"

6,7
-----------------------------27635325922190
Content-Disposition: form-data; name="Custom Date field"

01/30/2018
-----------------------------27635325922190
Content-Disposition: form-data; name="Custom Short text field"

Corto
-----------------------------27635325922190
Content-Disposition: form-data; name="Custom Long text field"

Largo
-----------------------------27635325922190
Content-Disposition: form-data; name="Custom Yes or not field"

Custom Yes or not field
-----------------------------27635325922190
Content-Disposition: form-data; name="Custom File upload field[0]"; filename="2018-01-22_14-13-36.png"
Content-Type: image/png

�PNG
�

Answer says: false

@Yajo

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2018

I added the possibility to add multiple model fields at once, but I didn't have time yet to check that bug @rafaelbn

@rafaelbn

This comment has been minimized.

Copy link
Member

commented Jan 25, 2018

Thanks @Yajo ! after reviewing the BUG it is 👍 in my side!

@Yajo

This comment has been minimized.

Copy link
Member Author

commented Jan 26, 2018

I can reproduce the bug if I try to add a file upload field when the model has no mail.thread features. I'm not sure if that's a bug or how to fix that, honestly, but I will add a note in known issues for now.

@Yajo

This comment has been minimized.

Copy link
Member Author

commented Jan 26, 2018

Done now @rafaelbn

@rafaelbn

This comment has been minimized.

Copy link
Member

commented Jan 26, 2018

@Yajo stil fails in model crm.lead (for ex.) basic one 😄

@Yajo

This comment has been minimized.

Copy link
Member Author

commented Jan 29, 2018

Could you please try in the empty database? It seems to be conflicting with some other addon, maybe one that validates input or something like that. Your very same form works for me locally.

@rafaelbn

This comment has been minimized.

Copy link
Member

commented Feb 2, 2018

That's right @Yajo I tested in production in customer and it works. 👍 😄

@rafaelbn
Copy link
Member

left a comment

Great one!

@rafaelbn rafaelbn merged commit 94c9f12 into OCA:10.0 Feb 2, 2018

4 checks passed

ci/runbot runbot build 3315900-402-d86864 (runtime 618s)
Details
codecov/patch Coverage not affected when comparing 28910f7...d868642
Details
codecov/project 84.55% remains the same compared to 28910f7
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pedrobaeza pedrobaeza deleted the Tecnativa:10.0-website_form_builder branch Feb 2, 2018

@Yajo

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2018

Big milestone today! 🚀 🎉 😊

Daemo00 added a commit to Daemo00/website that referenced this pull request Jun 5, 2018

[10.0][ADD] website_form_builder: Exactly what the title says (OCA#402)
* [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

pedrobaeza added a commit to Daemo00/website that referenced this pull request Jun 28, 2018

[10.0][ADD] website_form_builder: Exactly what the title says (OCA#402)
* [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

hveficent added a commit to Eficent/website that referenced this pull request Oct 16, 2018

[10.0][ADD] website_form_builder: Exactly what the title says (OCA#402)
* [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

Tardo added a commit to Tecnativa/website that referenced this pull request May 16, 2019

[10.0][ADD] website_form_builder: Exactly what the title says (OCA#402)
* [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says

* fixup! [ADD] website_form_builder: Exactly what the title says
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.