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] crm_claim_ref_smartbutton: Smart-button for referenced claims #38

Closed
wants to merge 5 commits into from

Conversation

pedrobaeza
Copy link
Member

Smart-button for referenced claims

This module adds an smart-button in the models that can be referenced in a
claim with the number of linked claims and a shortcut to them.

Usage

Go to any record that can be referenced in a claim, and you will see in a
smart-button called linked claims a number indicating the number of
claims that reference this record. Clicking on the button, it will navigate
to a list of these claims.

If you click New on the claim list, the created claim will be automatically
linked to the referenced record.

Known issues / Roadmap

  • The check for seeing if the opened model is one of the possible referenced
    models is done on each view request, so this hits very little, but a bit,
    the overall performance of the system.

@pedrobaeza pedrobaeza force-pushed the 8.0-crm_claim_ref_smartbutton branch from 4179aa2 to 472b5b6 Compare July 31, 2015 17:59
@pedrobaeza
Copy link
Member Author

@StefanRijnhart, I think you would enjoy this piece of code.

Smart-button for referenced claims
==================================

This module adds an smart-button in the models that can be referenced in a
claim with the number of linked claims and a shortcut to them.

Usage
=====

Go to any record that can be referenced in a claim, and you will see in a
smart-button called _linked claims_ a number indicating the number of
claims that reference this record. Clicking on the button, it will navigate
to a list of these claims.

If you click _New_ on the claim list, the created claim will be automatically
linked to the referenced record.

Known issues / Roadmap
======================

* The check for seeing if the opened model is one of the possible referenced
  models is done on each view request, so this hits very little, but a bit,
  the overall performance of the system.
@pedrobaeza pedrobaeza force-pushed the 8.0-crm_claim_ref_smartbutton branch from 472b5b6 to 32972d6 Compare August 1, 2015 11:42
@anajuaristi
Copy link

Tested functionally on runbot, it works
👍 RTM (ready to merge) from my functional point of view.
Very usefull module

@oihane
Copy link
Contributor

oihane commented Aug 3, 2015

👍
Although coveralls is failing.

@pedrobaeza
Copy link
Member Author

There's an error opening "My dashboard" menu entry that it's solved in this PR to Odoo: odoo/odoo#7855

@pedrobaeza
Copy link
Member Author

Patch merged on Odoo, so everything is OK!

@pedrobaeza
Copy link
Member Author

Added tests!

@oihane
Copy link
Contributor

oihane commented Aug 4, 2015

@pedrobaeza tests are failing

self.assertTrue(
xml_field, "Smart-button not added to %s view" % model_name)
# Test the smart-button existence for a model not linked
for model in self.env['ir.model'].search():
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you have to put [] in order to search every model?

@pedrobaeza pedrobaeza force-pushed the 8.0-crm_claim_ref_smartbutton branch from 3118753 to 19afa24 Compare August 4, 2015 13:31
@pedrobaeza
Copy link
Member Author

Sorry, unfinished code was submitted.

],
"category": "Customer Relationship Management",
"installable": True,
"pre_init_hook": "pre_init_hook",
Copy link
Sponsor

Choose a reason for hiding this comment

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

@pedrobaeza hooks are only called at installation time not each time the server start. Therefore your monkey-patch will be only applied once. IOW your patch will no more be applied after the first time your sever will be restarted after the installation. odoo/odoo@4105b5f#diff-9f65b6631806b04e785c2acdf3854285R151

Copy link
Sponsor

Choose a reason for hiding this comment

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

@pedrobaeza The solution is may be to create a fake model and implement the _register_hook method to apply your monkey patch each time the module is loaded in the registry at startup time. https://github.com/odoo/odoo/blob/8.0/openerp/modules/loading.py#L446

Copy link
Member Author

Choose a reason for hiding this comment

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

You're only seeing half of the solution. This is the possible scenarios:

Copy link
Sponsor

Choose a reason for hiding this comment

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

@pedrobaeza Sorry for the noise.. haven't see the full solution. IMO, the solution can be simplified by monkey-patching in a _register_hook BaseModel in a fake non stored Model. In this way we no more need the pre_init_hook and monkey patching is done in one place.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you draft the proposition with the fake model? Take into account that this module can cover any linkable model in claims.

Copy link
Sponsor

Choose a reason for hiding this comment

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

class CrmClaimMonkeyPatcher(models.TransientModel):
    _name = 'cr.claim.mockey.patcher'

    _auto = False  # doesn't create table in database

    def _register_hook(self, cr):
        BaseModel.base_fields_view_get = base_fields_view_get
        BaseModel.fields_view_get = fields_view_get_crm_claim

@pedrobaeza pedrobaeza force-pushed the 8.0-crm_claim_ref_smartbutton branch 5 times, most recently from cb576be to 2984d91 Compare August 22, 2015 00:37
@pedrobaeza
Copy link
Member Author

I have followed @lmignon's advice and implement the monkey-patching in a model with _auto=False and now the monkey-patching is only done when the module is installed, and now is multi-db aware thanks to a list that registers the dbs where the module is installed.

@pedrobaeza pedrobaeza force-pushed the 8.0-crm_claim_ref_smartbutton branch from 2984d91 to dd1d789 Compare August 22, 2015 00:38
@pedrobaeza
Copy link
Member Author

@antespi you can check now

Smart-button for referenced claims
==================================

This module adds an smart-button in the models that can be referenced in a
Copy link
Member

Choose a reason for hiding this comment

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

s/ an / a /

@pedrobaeza
Copy link
Member Author

All remarks are solved, so this is ready to be merged.

@hbrunn
Copy link
Member

hbrunn commented Sep 8, 2015

@pedrobaeza nice solution. But the full table scan on res_request_link for every view load (can't this be ormcached?) bothers me, as well as the unconditional monkey patches.

How about monkey patching only existing models' fields_view_get that show up in res.request.link, and also monkey patch https://github.com/OCA/OCB/blob/8.0/openerp/modules/registry.py#L486 in _register_hook in order to collect new models popping up which are in res.request.link? Then you can do the work in fields_view_get unconditionally.

Then some details like adding/removing the patch in res.request.link#create/unlink

@hbrunn hbrunn added the question label Sep 8, 2015
@rafaelbn
Copy link
Member

rafaelbn commented Oct 7, 2015

@pedrobaeza I think you should answer @hbrunn and I will make a test in runbot in order to merge. OK?

@rafaelbn
Copy link
Member

Tested un runbot 👍

@anajuaristi
Copy link

Hi, could we merge this one? I think it could be very usefull have it available to use instead of staying as PR.

@pedrobaeza
Copy link
Member Author

@anajuaristi, there's already some suggestions on how to implement certain things that I haven't applied yet due to lack of time, so better to stay as a PR yet.

@StefanRijnhart
Copy link
Member

Agree with @hbrunn mainly about the monkeypatching (that will 'affect' instances with this module sitting uninstalled in the module path. Setting to WIP as per @pedrobaeza's last comment.

@StefanRijnhart
Copy link
Member

BTW. can a project admin create milestones for the older releases, so that we don't have to use the tag?

@max3903 max3903 modified the milestone: 8.0 Mar 10, 2016
@pedrobaeza
Copy link
Member Author

cc @Tecnativa

from lxml import etree


base_fields_view_get = models.BaseModel.fields_view_get
Copy link
Member

Choose a reason for hiding this comment

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

Why the monkeypatch?

Did you know you can inherit BaseModel Normally right?

BTW, due to this has an static folder this code will try to be executed.

Can you try do the feature without monkeypatch?

@nhomar
Copy link
Member

nhomar commented Jun 6, 2016

IOH: This kind of changes without tests (changes that modifiy the general behavior of odoo) are really dangerous, I prefer some minimal test cases .

@nhomar
Copy link
Member

nhomar commented Jun 6, 2016

Monkeypatched for me is a 👎 but it is a 👍 for the functionality.

@pedrobaeza
Copy link
Member Author

It's a WIP still, @nhomar. I will use another technique.

@rafaelbn
Copy link
Member

This is a WIP, @pedrobaeza are you going to continue it?

Bugs are tracked on `GitHub Issues <https://github.com/OCA/crm/issues>`_.
In case of trouble, please check there if your issue has already been reported.
If you spotted it first, help us smashing it by providing a detailed and welcomed feedback
`here <https://github.com/OCA/crm/issues/new?body=module:%20crm_claim_ref_smartbutton%0Aversion:%208.0%0A%0A**Steps%20to%20reproduce**%0A-%20...%0A%0A**Current%20behavior**%0A%0A**Expected%20behavior**>`_.
Copy link
Member

Choose a reason for hiding this comment

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

Please update with beautier newer template 😊

@rafaelbn
Copy link
Member

rafaelbn commented Oct 4, 2016

I guess you would like to finish this PR but will it be possible? Anyone can take this work and try to finish it?

@pedrobaeza pedrobaeza closed this Aug 16, 2017
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