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

[ADD] basic modules + proper README #1

Closed
wants to merge 0 commits into from
Closed

Conversation

simahawk
Copy link
Contributor

@simahawk simahawk commented Oct 3, 2016

Well, the title it's self-explaining :)

TODO

  • get rid of unicode strings
  • add missing translations in controllers (status info title, template exception, etc)
  • move status access messages to methods to be evaluated at runtime
  • security.mixin: use ir.rule for security checks plus computed fields for view/edit permissions
  • website_cms/controllers/main.PageViewController.view_page: rely on ir.rule instead of checking publisher
  • website_cms/controllers/main.TagsController.tags_search: check if =ilike is needed
  • media mimetype retrieval: check how odoo does it and reuse if possible (see if mimetype in VIDEO_TYPES) or simply rely on categories mimetype setup
  • media mimetype retrieval: use plain like and drop format
  • update_published -> toggle_published
  • load preview from youtube: move to_embed_url to method
  • media: use api.multi for is_image and is_video
  • media category: use api.multi for public_slug
  • move SIDEBAR_VIEW_DOMAIN to method
  • drop attachment_ids on cms page
  • page_type and page_type.id or False and alike: just return the browse object -> .id return false if no obj, see also [ADD] basic modules + proper README #1 (comment)
  • check/fix/improve ormcache usage (see path computation etc)
  • rethink about path stuff: get rid of it for building urls etc, see [ADD] basic modules + proper README #1 (comment)
  • rewrite checks like self.env.user.has_group('website_cms.cms_manager') to check for actual read/write permission instead of checking for groups, see [ADD] basic modules + proper README #1 (comment)
  • rewrite translations retrieval, see [ADD] basic modules + proper README #1 (comment)
  • check sudo usage [ADD] basic modules + proper README #1 (comment)
  • add a flag to page.type to determine if we can add sub pages to it, see [ADD] basic modules + proper README #1 (comment)
  • use unicode string with .format see [ADD] basic modules + proper README #1 (comment)
  • redirect.mixin: use api.multi
  • redirect.mixin: translate computed name
  • rewrite search query into website_cms_search, see [ADD] basic modules + proper README #1 (comment)
  • update menu structure and backend views, see [ADD] basic modules + proper README #1 (review)
  • move important TODOs to ROADMAP (like handling errors in frontend forms)
  • check and fix copyrights etc in all files
  • update README, CHANGELOG, etc
  • docs: publish them w/ github

@coveralls
Copy link

coveralls commented Oct 3, 2016

Coverage Status

Coverage remained the same at 53.014% when pulling 239a24b on simahawk:9.0 into ef6d55e on OCA:9.0.

@coveralls
Copy link

coveralls commented Oct 3, 2016

Coverage Status

Coverage remained the same at 53.014% when pulling 239a24b on simahawk:9.0 into ef6d55e on OCA:9.0.

@simahawk
Copy link
Contributor Author

simahawk commented Oct 3, 2016

need to fix some issues

@coveralls
Copy link

Coverage Status

Coverage remained the same at 53.385% when pulling 4678eb2 on simahawk:9.0 into ef6d55e on OCA:9.0.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 53.385% when pulling 4678eb2 on simahawk:9.0 into ef6d55e on OCA:9.0.

Bug Tracker
===========

Bugs are tracked on `GitHub Issues <https://github.com/OCA/CMS/issues>`_.
Copy link
Member

Choose a reason for hiding this comment

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

wrong url


Read the `contributors list`_

.. _contributors list: ./AUTHORS
Copy link
Member

Choose a reason for hiding this comment

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

Is this a new guideline? Don't you prefer a contributors list per addon?

Copy link

Choose a reason for hiding this comment

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

Please, follow OCA guideline for contributors, because this URL doesn't work when you are reading this README in Odoo Apps section.

Copy link

Choose a reason for hiding this comment

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

Indeed

Choose a reason for hiding this comment

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

Please follow the guidelines.


OCA, or the Odoo Community Association, is a nonprofit organization whose mission is to support the collaborative development of Odoo features and promote its widespread use.

To contribute to this module, please visit http://odoo-community.org.
Copy link
Member

Choose a reason for hiding this comment

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

https x3

* Introduce portlets for sidebar elements
* Add "collections" to fetch contents from the whole website (eg: "News from 2016")
* Improve test coverage
* Default theme
Copy link
Member

Choose a reason for hiding this comment

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

Aren't roadmaps better per addon?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pedrobaeza asked me for that :)

Copy link
Member

Choose a reason for hiding this comment

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

Both are needed, @yajo. A general one in the README of the repository to see the goal of it, and a specific roadmap inside the module.

Copy link

Choose a reason for hiding this comment

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

I personally see this as a maintenance problem. I would definitely never know to look here, or update here.

Copy link
Member

Choose a reason for hiding this comment

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

It seems more a maintenance burden than a help to put this file, but nevermind.

Copy link

Choose a reason for hiding this comment

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

Couldn't we instead utilize the Wiki for the overall roadmap?

Copy link
Member

Choose a reason for hiding this comment

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

Or better an issue with check boxes

@@ -0,0 +1,6 @@
# -*- coding: utf-8 -*-
# © <YEAR(S)> <AUTHOR(S)>
Copy link
Member

Choose a reason for hiding this comment

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

Missing (and in other places, but I'll apply DRY)

_description = "A mixin for protecting website content"
# admin groups that bypass security checks
_admin_groups = (
'base.group_website_publisher',
Copy link
Member

Choose a reason for hiding this comment

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

use perms, not groups

Bypass if:

* you are super-user
* you are in a group listed in `_admin_groups`
Copy link
Member

Choose a reason for hiding this comment

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

you know...

return True

# obj comes w/ a temporary env,
# w/ uid beeing an instance of ir_http.RequestUID
Copy link
Member

Choose a reason for hiding this comment

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

being

# which is not suitable for std ORM operations.
# Let's make sure we get the right user here!
if request.session.uid:
_obj = obj.with_env(self.env(user=request.session.uid))
Copy link
Member

Choose a reason for hiding this comment

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

obj.sudo(request.session.uid)

Disclaimer: yes, we could use `ir.rule`s for this
but since we want the end user to select a group
easily without having to get in touch w/ rules,
we chosed this way.
Copy link
Member

Choose a reason for hiding this comment

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

If the group is attached to some record, then you can add a ir.rule like [("somefield", "in", user.group_id)] and remove lots of code

@coveralls
Copy link

coveralls commented Oct 3, 2016

Coverage Status

Coverage remained the same at 53.385% when pulling 3da6e27 on simahawk:9.0 into ef6d55e on OCA:9.0.

"GROUP BY {ob_col}"
).format(ob_col=ob_col,
relation=relation,
group_col=group_col)
Copy link
Member

Choose a reason for hiding this comment

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

🚫 SQL injection flaw.

@@ -0,0 +1,24 @@
<?xml version="1.0" encoding="utf-8"?>
<openerp>
<data>
Copy link
Member

Choose a reason for hiding this comment

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

<odoo>

@@ -0,0 +1,16 @@
<?xml version="1.0" encoding="utf-8"?>
<openerp>
<data>
Copy link
Member

Choose a reason for hiding this comment

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

<odoo>

@@ -0,0 +1,134 @@
<?xml version="1.0" encoding="utf-8"?>
<openerp>
<data>
Copy link
Member

Choose a reason for hiding this comment

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

<odoo>

See http://select2.github.io/select2/#initSelection -->
<input type="text" name="tag_ids" class="form-control js_select2" style="clear:both"
t-att-data-can-create="is_cms_manager and 1 or None"
placeholder="Tags" t-attf-data-init-value="#{tag_ids}" value="tags go here" />
Copy link
Member

Choose a reason for hiding this comment

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

Do you really want that value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, otherwise is not going to work, as commented a few lines above.


<div class="form-group mt32">
<div class="pull-right">
<a class="btn btn-default" t-att-href="request.httprequest.referrer">Cancel</a>
Copy link
Member

Choose a reason for hiding this comment

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

What if no referrer?

Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

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

Also please remove website_cms_search/tests/__init__.py

# we need this check here, since ir.ui.view (and potentially other models)
# do not have `website_published` attribute that is used
# by management actions for publishing/unpublishing contents.
</t>
Copy link
Member

@yajo yajo Oct 4, 2016

Choose a reason for hiding this comment

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

Just put a comment, it gets stripped out in the final output.

@@ -0,0 +1,48 @@
<?xml version="1.0" encoding="utf-8"?>
<openerp>
<data>
Copy link
Member

Choose a reason for hiding this comment

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

odoo


<!-- edit / translate buttons have weird conditions: protect them better.
For instance, the homepage controller overrides `editable` and `translatable`
to allow only `bwi_website.group_homepage_manager` group to update it. -->
Copy link
Member

Choose a reason for hiding this comment

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

what?

<li class="edit edit-frontend ml16">
<a title="Edit metadata (name, description, etc)" class="btn btn-link btn-xs"
t-att-href="website.cms_edit_link(main_object=main_object)">
<i class="fa fa-pencil-square-o" /> Metadata</a>
Copy link
Member

Choose a reason for hiding this comment

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

Use <div> for fontawesome icons.
The final result is the same, but <i> tags get included in translations from v9 onwards. This way you will get a cleaner .pot file later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you cannot do it for accessibility, see http://stackoverflow.com/questions/24360199/can-we-place-a-div-inside-an-a-tag-according-to-the-accessibility

I'll check which tag I can use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you have t- attributes the element is ignored, see here. I think I can use t-ignore.

Copy link
Member

Choose a reason for hiding this comment

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

You should use t-translation="off" instead.

<li class="edit edit-backend" groups="website_cms.cms_manager">
<a title="Edit in backend (do everything trough backend)" class="btn btn-link btn-xs"
t-att-href="website.cms_edit_backend_link(main_object=main_object)">
<i class="fa fa-pencil-square-o" /> Backend</a>
Copy link
Member

Choose a reason for hiding this comment

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

use div

AND tr.res_id=p.id
AND tr.name like 'cms.page,%%'
AND tr.state='translated'
""".format(lang)
Copy link
Member

@yajo yajo Oct 4, 2016

Choose a reason for hiding this comment

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

Always use unicode strings for format.

if case_sensitive:
sql_query += """ AND tr.value {like} %s"""
else:
sql_query += """ AND lower(tr.value) {like} %s"""
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you prefer to use the Query class.

order = 'published_date desc'
case_sensitive = self._case_sensitive
sql_query = self._get_query(lang, case_sensitive=case_sensitive)
params = ['%{}%'.format(search_text)
Copy link
Member

Choose a reason for hiding this comment

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

unicode here

page_ids = tuple(set([x[0] for x in res]))

# limit = self._results_per_page
# offset = (page - 1) * self._results_per_page
Copy link
Member

Choose a reason for hiding this comment

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

💩

sql_query = self._get_query(lang, case_sensitive=case_sensitive)
params = ['%{}%'.format(search_text)
for x in xrange(sql_query.count('%s'))]
request.env.cr.execute(sql_query, params)
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, probably above comments make no real sense in the way that... all of this makes no sense.

Did you know that the ORM already does all the translation stuff automatically? What you are doing here is basically the same as request.env["cms.page"].search(["|","|",("name", "ilike", search_text), ("description", "=", search_text), ("body", "ilike", search_text)])?

BTW, why the case_sensitive thing? Who would ever want a case sensitive search?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll check this.

@@ -0,0 +1,7 @@
CHANGELOG

Choose a reason for hiding this comment

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

this should be in README direct


Read the `contributors list`_

.. _contributors list: ./AUTHORS

Choose a reason for hiding this comment

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

Please follow the guidelines.

pages = self.search(domain, order=order)
return pages

def pager(self, total, page=1, step=10,
Copy link
Member

Choose a reason for hiding this comment

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

I think we should simply drop this and use the default pager instead, which BTW is compatible with website_canonical_url by default.

Copy link

@antespi antespi left a comment

Choose a reason for hiding this comment

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

This are the changes commented with @simahawk in OCA Sprint in Belgium

  • Make other views for common use cases, as a startup sample:
    • Image/video gallery
    • Related pages
  • Move CMS Admin menu to Website Admin menu:
    • Websites
      • Websites: tree view over website object
    • Content
      • Menus: tree view over website.menu groupby website_id by default
      • Pages: tree view over ir.ui.view with domain [('page', '=', True)]
      • Layouts: tree view over ir.ui.view with domain [('cms_view', '=', True)]
      • Contents: tree view over cms.page


Read the `contributors list`_

.. _contributors list: ./AUTHORS
Copy link

Choose a reason for hiding this comment

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

Please, follow OCA guideline for contributors, because this URL doesn't work when you are reading this README in Odoo Apps section.

@@ -0,0 +1,5 @@
.DS_Store
Copy link

@lasley lasley Oct 4, 2016

Choose a reason for hiding this comment

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

@@ -0,0 +1 @@
* Simone Orsi at Camptocamp
Copy link

@lasley lasley Oct 4, 2016

Choose a reason for hiding this comment

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

Please remove this file and add to the contributors section of module readme

@@ -0,0 +1,7 @@
CHANGELOG
Copy link

Choose a reason for hiding this comment

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

Please remove this file, maintenance nightmare


Documentation: TODO

Premise
Copy link

Choose a reason for hiding this comment

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

Please follow the template readme - https://github.com/OCA/maintainer-tools/blob/master/template/module/README.rst

Additionally, and IMO, describing features that Odoo lacks is not helpful and irrelevant. You should instead describe what this module does.


Read the `contributors list`_

.. _contributors list: ./AUTHORS
Copy link

Choose a reason for hiding this comment

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

Indeed

if status_message:
self.add_status_message(status_message)

# def get_fields(self, writable_fields):
Copy link

Choose a reason for hiding this comment

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

Please remove useless code

self.before_post_action(parent=parent, **kw)
# handle form submission
values = self.load_defaults(parent, **kw)
# TODO: handle errors
Copy link

Choose a reason for hiding this comment

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

Please add this to roadmap

self._check_security(main_object)
self.before_post_action(main_object=main_object, **kw)
# handle form submission
# TODO: handle errors
Copy link

Choose a reason for hiding this comment

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

Add to roadmap

msg = {
'type': 'warning',
'title': 'Note:',
'msg': _(u'No description for this page yet. '
Copy link

Choose a reason for hiding this comment

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

Why unicode?

template = view_item.view_id.key

if not template:
raise NotImplementedError("You must provide a template!")
Copy link

Choose a reason for hiding this comment

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

Translate?

@lasley
Copy link

lasley commented Oct 4, 2016

Admittedly only finished half the review, but I figured it would be worth submitting and coming back to in a bit

@coveralls
Copy link

coveralls commented Oct 4, 2016

Coverage Status

Coverage remained the same at 53.385% when pulling 67a6040 on simahawk:9.0 into ef6d55e on OCA:9.0.

@coveralls
Copy link

coveralls commented Oct 4, 2016

Coverage Status

Coverage remained the same at 53.385% when pulling 67a6040 on simahawk:9.0 into ef6d55e on OCA:9.0.


@http.route('/cms/get_tags', type='http',
auth="public", methods=['GET'], website=True)
def tags_search(self, q='', l=25, **post):
Copy link

Choose a reason for hiding this comment

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

I think you can drop this controller method and use JSON-RPC API for searching tags.

Copy link

Choose a reason for hiding this comment

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

Note that using JSON RPC will disallow the public user from using this application. This may be a hinderance to some usage, thus I would say we should probably leave the controller until I finish OCA/web#402

Copy link

Choose a reason for hiding this comment

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

You're right @lasley

}
},
ajax: {
url: '/cms/get_tags',
Copy link

Choose a reason for hiding this comment

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

I think you can drop this custom method and use JSON-RPC API for searching tags, as explained here: https://www.odoo.com/documentation/9.0/reference/javascript.html#high-level-api-calling-into-odoo-models

Copy link

Choose a reason for hiding this comment

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

Same note re public user session

Copy link

Choose a reason for hiding this comment

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

OK

@coveralls
Copy link

coveralls commented Oct 5, 2016

Coverage Status

Coverage remained the same at 53.385% when pulling 589369f on simahawk:9.0 into ef6d55e on OCA:9.0.

# # published
# # self.f2.website_published = True
# # self.f2.public = True
# # self.f2.invalidate_cache()
Copy link
Member

Choose a reason for hiding this comment

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

💩

<filter
name="only_main"
string="Only main"
icon="terp-personal"
Copy link
Member

Choose a reason for hiding this comment

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

seems legit

@coveralls
Copy link

coveralls commented Oct 11, 2016

Coverage Status

Coverage remained the same at 68.665% when pulling 2da2897 on simahawk:9.0 into ef6d55e on OCA:9.0.

@coveralls
Copy link

coveralls commented Oct 11, 2016

Coverage Status

Coverage remained the same at 68.665% when pulling 453a271 on simahawk:9.0 into ef6d55e on OCA:9.0.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 68.726% when pulling 88b3890 on simahawk:9.0 into ef6d55e on OCA:9.0.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 68.726% when pulling 88b3890 on simahawk:9.0 into ef6d55e on OCA:9.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.

None yet

7 participants