Skip to content

Conversation

@obulkin
Copy link

@obulkin obulkin commented Nov 18, 2016

Help Needed: Ideally, the web.kanban.abstract.tester model should only be loaded during testing. Does anyone know how to do this?

This PR mostly repackages the code in OCA/vertical-medical#121 and was also motivated by a WIP PR by @dreispt: OCA/server-tools#368.

  • Add Kanban-compatible stage model web.kanban.stage
  • Add views, menu items, actions, and access controls needed to manage web.kanban.stage records
  • Add abstract model web.kanban.abstract that other models can inherit from to gain Kanban stage functionality
  • Add base Kanban view web_kanban_abstract_view_kanban, which can be customized as needed for use with models that inherit from web.kanban.abstract
  • Add model web.kanban.abstract.tester, which is needed for web.kanban.abstract unit tests

* Add Kanban-compatible stage model web.kanban.stage
* Add views, menu items, actions, and access controls needed to manage
web.kanban.stage records
* Add abstract model web.kanban.abstract that other models can inherit from to
gain Kanban stage functionality
* Add base Kanban view web_kanban_abstract_view_kanban, which can be
customized as needed for use with models that inherit from web.kanban.abstract
* Add model web.kanban.abstract.tester, which is needed for
web.kanban.abstract unit tests
Copy link
Contributor

@lasley lasley left a comment

Choose a reason for hiding this comment

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

Thanks @obulkin - looks great outside of some minor comments. I'm happy to see our medical logic finally becoming useful in other contexts

------------

* Dave Lasley <dave@laslabs.com>
* Oleg Bulkin <obulkin@laslabs.com>
Copy link
Contributor

@lasley lasley Nov 18, 2016

Choose a reason for hiding this comment

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

Add * Daniel Reis - while he didn't write any of this particular code, I made some alterations to the medical library based off of his OCA/server-tools#368. Seems only right to add him into contrib IMO

@@ -0,0 +1,107 @@
.. image:: https://img.shields.io/badge/licence-AGPL--3-blue.svg
:target: http://www.gnu.org/licenses/agpl-3.0-standalone.html
:alt: License: AGPL-3
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's license this LGPL. Easy way to change is just replace in directory AGPL for LGPL and agpl for lgpl


class WebKanbanAbstract(models.AbstractModel):
'''Inherit from this class to add support for Kanban stages to your model.
All public properties are preceded with kanban_ in order to isolate from
Copy link
Contributor

Choose a reason for hiding this comment

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

Add an extra carriage return between the first description line & this extended info. Take a look at https://google.github.io/styleguide/pyguide.html & go to the comments section. But really this whole guide is a great extension to PEP-8 & PEP-257

@lasley lasley added this to the 9.0 milestone Nov 18, 2016
@lasley
Copy link
Contributor

lasley commented Nov 18, 2016

I added the question tag for your question regarding the loading of the test model only during testing, because I do not have an idea here. I think @hbrunn performed some wizardry like this in one of his modules, but I can't remember which it was.

@simahawk
Copy link
Contributor

+1 for the question! It's something that we definitely want for testing. Looks like Odoo team creates test_* modules to register new models, but in normal day-life is overwhelming and not what you really want.

@dreispt
Copy link
Member

dreispt commented Nov 18, 2016

This should be a 'base' module, and not a 'web' module.
web modules add web client features (mostly js/CSS).

@hbrunn
Copy link
Member

hbrunn commented Nov 18, 2016

for the tests https://github.com/OCA/server-tools/blob/9.0/database_cleanup/tests/test_database_cleanup.py#L55 should be helpful. But note that this code has its deficiencies and is about to be fixed in OCA/server-tools#612

@lasley
Copy link
Contributor

lasley commented Nov 19, 2016

@dreispt so base_kanban_stage then? I think it should still be in this repo though, vs server-tools

@lasley
Copy link
Contributor

lasley commented Nov 19, 2016

Brilliant @hbrunn thank you. This is totally what we needed, and it's so danged simple too.

@obulkin & @simahawk key is self.env['ir.model'].create({, then using the inherited_model_ids to define our inheritance from the abstract model.

@dreispt
Copy link
Member

dreispt commented Nov 19, 2016

It is not a web client feature.
Why should it belong to the web repo ?

@lasley
Copy link
Contributor

lasley commented Nov 19, 2016

It extends web_kanban, which extends web. Seems to me that it is most certainly a web client feature, unless we have a repo dedicated to web_kanban. Don't the features in server-tools all just extend from base?

@dreispt
Copy link
Member

dreispt commented Nov 19, 2016

I don't think it extends the web client kanban view type or widgets. Technically the web_kanban dependency is not needed and can be removed.
Even though the point is to make use of the features added on Kanban boards.

@lasley
Copy link
Contributor

lasley commented Nov 19, 2016

We provide an abstract KanBan view here. I'm pretty sure this module would break without the web_kanban module due to the linked line.

@obulkin
Copy link
Author

obulkin commented Nov 22, 2016

Thanks for the suggestion, @hbrunn. self.env['ir.model'].create() works for getting the test model up, but it also somehow breaks the containment provided by TransactionCase, so that the resulting ir.model record persists after the test, the new model remains accessible using self.env['model_name'], and so on. Any ideas on how to deal with this? The problem is obscured in the example that you provided because the code that's being tested there is specifically meant to address scenarios like this.

@hbrunn
Copy link
Member

hbrunn commented Nov 22, 2016

to be honest, I never worried about that because here, I only run tests on databases meant to throw away. But it's not beautiful of course. Just do what the create function does anyways, but without whatever function that does a commit.
Have a look at https://github.com/OCA/bank-statement-import/blob/10.0/account_bank_statement_import_save_file/tests/test_save_file.py#L29

@moylop260
Copy link

Thanks for the suggestion, @hbrunn. self.env['ir.model'].create() works for getting the test model up, but it also somehow breaks the containment provided by TransactionCase, so that the resulting ir.model record persists after the test, the new model remains accessible using self.env['model_name'], and so on. Any ideas on how to deal with this? The problem is obscured in the example that you provided because the code that's being tested there is specifically meant to address scenarios like this.

I fixed that for database_cleanup from OCA/server-tools#612
You could see tearDown method.

@moylop260
Copy link

moylop260 commented Nov 23, 2016

...the explanation is: OCA/server-tools#612 (comment)
I missed a update of environment models, but the code is:self.registry.setup_models(self.env.cr, partial=False)

@obulkin
Copy link
Author

obulkin commented Nov 23, 2016

Ah, interesting. Thanks for the added input, @moylop260 and @hbrunn. I was able to resolve the issue by using a combination of the suggested techniques, together with a tweak: https://OCA/web/pull/490/files#diff-724d82a36bc47b86e2023141c932a42fR9

The key was adding 'state': 'base' to the self.env['ir.model'].create() call. This creates an ir.model record without the calls at https://github.com/odoo/odoo/blob/9.0/openerp/addons/base/ir/ir_model.py#L187-L188, which involve multiple commits and hence breach the test transaction.

I've pushed up a new commit with this change, as well as the ones requested by @lasley, so the PR is ready for more review.

Copy link
Contributor

@lasley lasley left a comment

Choose a reason for hiding this comment

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

Changes LGTM thanks @obulkin

@dreispt (or anyone else) - regarding #490 (comment) - do we want to rename this, or change repos, even if it is dependent on web_kanban and provides the default kanban view?

@dreispt
Copy link
Member

dreispt commented Nov 27, 2016

That abstract Kanban is a view accelerator to ease using the features provided.
I expect a 'web' module to extend web client features, and to be mostly about front end Js/CSS, and corresponding server side controllers.
But this module is mostly about a reusable backend feature.
It is still my opinion that it comes unexpected for this repo to host this feature, and the proper place would be server-tools.
But maybe I'm the only one with that opinion.

@lasley
Copy link
Contributor

lasley commented Dec 2, 2016

@dreispt - Alright so it seems nobody else has an opinion. I'm going to go ahead and yield to your's then - you've been around longer than me so I'd say your repo judgement is probably better than mine.

@obulkin - Please move this to server-tools

@dreispt
Copy link
Member

dreispt commented Dec 2, 2016 via email

@simahawk
Copy link
Contributor

simahawk commented Dec 4, 2016

Hmm, my POV is that kanban is a web component. Is a web client widget. The fact that this module does not provide any JS/CSS etc does not mean that is not web related stuff.

@dreispt today is a backend view but in the future with v11 whereas the widgets will be decoupled from backend views... kanban and many other client features could be used on the frontend too ;)
My $0.02

@dreispt
Copy link
Member

dreispt commented Dec 4, 2016

@simahawk my concern is that no features are being added to Kanban Views,
It is rather an accelerator to implement the Kanban pattern on other Models.
Nothing specific to webclient development here.

@dreispt
Copy link
Member

dreispt commented Dec 4, 2016

@simahawk PS: I'm curious about your "v11 widgets will be decoupled from backend views" comment. Can you share more about it?

@simahawk
Copy link
Contributor

simahawk commented Dec 4, 2016 via email

@pedrobaeza
Copy link
Member

My colleague @yajo had the same talk and we are eager to see that.

@lasley
Copy link
Contributor

lasley commented Dec 17, 2016

Does anyone else have thoughts on where this module should be located?

@dreispt
Copy link
Member

dreispt commented Dec 19, 2016

I would really like for this to be at server-tools as base_kanban_stage.
This does not implement grouping stages into states, as intended by base_stage_state, so they would be complementary features.

@lasley
Copy link
Contributor

lasley commented Dec 19, 2016

Seems like we're just kind of going back and forth on the stay leave, so I'm changing my vote to server-tools as well. @obulkin please process the repo move & name change.

@lasley
Copy link
Contributor

lasley commented Dec 28, 2016

Moved to OCA/server-tools#679

@lasley lasley closed this Dec 28, 2016
@lasley lasley deleted the feature/9.0/LABS-134-create-medical_base_stage-object branch December 28, 2016 19:10
@lasley lasley restored the feature/9.0/LABS-134-create-medical_base_stage-object branch December 28, 2016 19:12
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.

7 participants