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

[IMP] rating_*: Improvement #13692

Merged
merged 4 commits into from Jan 9, 2017

Conversation

jat-odoo
Copy link
Contributor

@jat-odoo jat-odoo commented Oct 6, 2016

Description of the issue/feature this PR addresses:

Current behavior before PR:

Desired behavior after PR is merged:

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

rating:-

  • Added in kanban:
    • link to related resource
    • Indicator to show feedback
    • Click on kanban open rating detail form view
    • Ellipsis for long string of customer and resource, So it fit on one line
  • Add tree view for rating

rating_project:-

  • Add rating filter on project's tasks

@jat-odoo jat-odoo force-pushed the master-rating-improvement-jat branch 2 times, most recently from 7f318ff to fd9d13e Compare October 6, 2016 12:42
@C3POdoo C3POdoo added the RD research & development, internal work label Oct 11, 2016
@rga-odoo rga-odoo force-pushed the master-rating-improvement-jat branch from fd9d13e to a2f8a51 Compare October 14, 2016 09:42
@jat-odoo jat-odoo force-pushed the master-rating-improvement-jat branch 2 times, most recently from 0a4752c to ca09307 Compare October 19, 2016 07:00
@jat-odoo jat-odoo force-pushed the master-rating-improvement-jat branch 3 times, most recently from 078e4da to 29555ed Compare October 26, 2016 07:29
@jat-odoo jat-odoo force-pushed the master-rating-improvement-jat branch from 29555ed to d6b090a Compare November 11, 2016 09:04
@jat-odoo jat-odoo force-pushed the master-rating-improvement-jat branch 2 times, most recently from 778b664 to 84dd27a Compare November 22, 2016 07:02
@jem-odoo jem-odoo force-pushed the master-rating-improvement-jat branch 7 times, most recently from fe2e96e to 1c8e80a Compare December 5, 2016 09:31
@jem-odoo jem-odoo force-pushed the master-rating-improvement-jat branch 2 times, most recently from 4af7349 to deab579 Compare December 7, 2016 14:08
@jem-odoo jem-odoo force-pushed the master-rating-improvement-jat branch 4 times, most recently from 7be684d to 7e303a9 Compare December 23, 2016 08:13
Copy link
Contributor

@tde-banana-odoo tde-banana-odoo left a comment

Choose a reason for hiding this comment

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

Quick technical review, but I am tired

res_model = fields.Char(string='Document Model', required=True, help="Model name of the rated object", index=True)
res_name = fields.Char(string='Resource name', compute='_compute_res_name', store=True, help="The name of the rated resource.")
res_model_id = fields.Many2one('ir.model', 'Related Document Model', index=True, ondelete='cascade', help='Model of the followed resource')
res_model = fields.Char(string='Document Model', related='res_model_id.model', store=True, index=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably add a readonly=True

@@ -19,10 +19,11 @@ def write(self, values):
return res

def _send_issue_rating_mail(self):
force_send = self.env.context.get('force_send', True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this context key used ? What is its purpose ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup

@jem-odoo jem-odoo force-pushed the master-rating-improvement-jat branch from 7e303a9 to 232d984 Compare December 30, 2016 14:42
Copy link
Contributor

@tde-banana-odoo tde-banana-odoo left a comment

Choose a reason for hiding this comment

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

Tech review

rating.rating_image = False

@api.multi
def _compute_rating_text(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing dependency

message_id = fields.Many2one('mail.message', string="Linked message", help="Associated message when posting a review. Mainly used in website addons.", index=True)
access_token = fields.Char('Security Token', default=new_access_token, help="Access token to set the rating of the value")
consumed = fields.Boolean(string="Filled Rating", help="Enabled if the rating has been filled.")

@api.multi
def _compute_rating_image(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

MIssing depedency

rating_count = fields.Integer('Rating count', compute="_compute_rating_count")

def write(self, values):
Copy link
Contributor

Choose a reason for hiding this comment

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

Write should be below computed fields ;)

@@ -19,10 +19,11 @@ def write(self, values):
return res

def _send_issue_rating_mail(self):
force_send = self.env.context.get('force_send', True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Yup

@jem-odoo jem-odoo force-pushed the master-rating-improvement-jat branch from 232d984 to 19f4815 Compare January 9, 2017 12:24
jat-odoo and others added 3 commits January 9, 2017 16:19
In rating module
- provide list and form view for rating
- better kanban view (remove js code, ellispsis
for long name, better feedback display, ...)
- provide more field on the mixin to cover
more case
- update res_name of rating when the ressource name
is changed (for consistency)
- make the rating web page, a correct html page (by
using web.layout template)
- some guidelines compliance

In rating_project and rating_project_issue:
- add filter for rated document
- some guidelines compliance
- use rating.mixin field instead of rating_ids to
display rating on form view
- clean controller method
- ...
Automatically deleting a rating was not implemented
yet. We find 2 cases :
- when uninstalling a module, the same mecanism as
mail.follower is used, via a res_model_id field
pointing on ir.model. When this model does not
exist anymore, remove all related rating.
- when deleting a record, its rating should be
deleted too.
@jem-odoo jem-odoo force-pushed the master-rating-improvement-jat branch from 5dca624 to 8e10f6a Compare January 9, 2017 15:33
@jem-odoo jem-odoo merged commit 8e10f6a into odoo:master Jan 9, 2017
@tde-banana-odoo tde-banana-odoo deleted the master-rating-improvement-jat branch January 20, 2017 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RD research & development, internal work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants