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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update views.rst to make color an required attribute #16671

Closed
wants to merge 4 commits into from

Conversation

fxkopp
Copy link
Contributor

@fxkopp fxkopp commented Apr 27, 2017

Description of the issue/feature this PR addresses:

Making the color an required property because a JavaScript file is using it in a "if"-statement.

web_calendar.js:799
if (color_field.type == "selection")

Current behavior before PR:

Throwing the following error:
Uncaught TypeError: Cannot read property 'type' of undefined in web_calendar.js:799

Desired behavior after PR is merged:

Odoo users will know how to avoid this error 馃憤

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

Setting a color is required because the web_calendar.js:799 is using "if (color_field.type == "selection")" so its throwing this error when color is not defined:

Uncaught TypeError: Cannot read property 'type' of undefined
@mart-e
Copy link
Contributor

mart-e commented Apr 27, 2017

Hello,

Instead of making it required in the doc, shouldn't we make the calendar work, even without the attribute?
I see a lot of if (this.color_field && in the code which makes me believe, we just forgot it somewhere.

@xmo-odoo xmo-odoo requested a review from Gorash April 27, 2017 12:34
@Gorash
Copy link
Contributor

Gorash commented May 3, 2017

Hi,

@mart-e look https://github.com/odoo/odoo/blob/10.0/addons/web_calendar/static/src/js/web_calendar.js#L791

When the flag useContacts is false, check that the field color_field exists.

fxkopp added 2 commits May 3, 2017 11:11
Commit 5fddac7 fixes this error, so color no longer has to be a required field.
@fxkopp
Copy link
Contributor Author

fxkopp commented May 3, 2017

Hi,

@mart-e and @Gorash I tried to fix this error. Would you mind taking a look and ping if there is anything else to be changed?

@Gorash
Copy link
Contributor

Gorash commented May 3, 2017

I did not test but the fix looks good 馃憤

@Gorash
Copy link
Contributor

Gorash commented May 8, 2017

Hi,
I modify the validation message to make it clearer.

Merged: 20facf5

Thank you for your contribution

@Gorash Gorash closed this May 8, 2017
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