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

[14.0][ADD] web_tree_customized_field_list #623

Merged
merged 1 commit into from
May 2, 2024

Conversation

ilyasProgrammer
Copy link
Member

@ilyasProgrammer ilyasProgrammer commented Mar 22, 2023

No description provided.

@elvise
Copy link

elvise commented Sep 27, 2023

@ilyasProgrammer can you rebase ?

@elvise
Copy link

elvise commented Sep 27, 2023

/ocabot rebase

@OCA-git-bot
Copy link
Contributor

Sorry @elvise you are not allowed to rebase.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

@ilyasProgrammer
Copy link
Member Author

@elvise done

@elvise
Copy link

elvise commented Dec 28, 2023

@ilyasProgrammer please rebase :)

@ilyasProgrammer ilyasProgrammer marked this pull request as ready for review February 7, 2024 14:15
@ilyasProgrammer ilyasProgrammer force-pushed the 14.0-customized_list_view branch 6 times, most recently from b5630bc to 131f3e1 Compare February 9, 2024 08:33
Copy link
Contributor

@francesco-ooops francesco-ooops left a comment

Choose a reason for hiding this comment

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

Functional ok!

Often a customer asks to add a field to tree view, and this requires dev time. This module allows functionals to be independent on adding fields to tree view.

Copy link

@geomer198 geomer198 left a comment

Choose a reason for hiding this comment

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

Could you please fix code by my comments.

customized_list_view/models/custom_list.py Outdated Show resolved Hide resolved
customized_list_view/models/custom_list.py Outdated Show resolved Hide resolved
customized_list_view/models/custom_list.py Outdated Show resolved Hide resolved
customized_list_view/models/custom_list.py Outdated Show resolved Hide resolved
customized_list_view/models/custom_list.py Outdated Show resolved Hide resolved
customized_list_view/models/custom_list.py Outdated Show resolved Hide resolved
customized_list_view/models/custom_list.py Outdated Show resolved Hide resolved
customized_list_view/models/custom_list.py Outdated Show resolved Hide resolved
customized_list_view/models/custom_list.py Outdated Show resolved Hide resolved
customized_list_view/tests/test_customized_list_view.py Outdated Show resolved Hide resolved
@ilyasProgrammer
Copy link
Member Author

@geomer198 Thanks for the review. Updated according to your suggestions.

Copy link

@aleuffre aleuffre left a comment

Choose a reason for hiding this comment

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

Code review, only a few minor remarks.

However, I should point out that the customizations do not survive an upgrade. In other words, when any module is upgraded, the views of that module are reverted back to what is written in the XML file, and it would be necessary to press "Apply Changes" again to make them appear again (Tested on runboat)

customized_list_view/models/custom_list.py Outdated Show resolved Hide resolved
customized_list_view/security/ir.model.access.csv Outdated Show resolved Hide resolved
@ilyasProgrammer
Copy link
Member Author

ilyasProgrammer commented Feb 15, 2024

Code review, only a few minor remarks.

However, I should point out that the customizations do not survive an upgrade. In other words, when any module is upgraded, the views of that module are reverted back to what is written in the XML file, and it would be necessary to press "Apply Changes" again to make them appear again (Tested on runboat)

Thats right. Updated description and added roadmap.

Copy link

@aleuffre aleuffre left a comment

Choose a reason for hiding this comment

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

Approved with the above caveat that it may be very counterintuitive to use.

class Module(models.Model):
_inherit = "ir.module.module"

def _button_immediate_function(self, function):

Choose a reason for hiding this comment

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

praise this is an elegant way of making the changes persistent

thought: not all changes may be applied. at any given time. I'd say we could either add the active field to the custom.list.view model, or another boolean field to keep track of which changes need to be applied and which ones should not.

@aleuffre
Copy link

Code review LGTM

Copy link

@geomer198 geomer198 left a comment

Choose a reason for hiding this comment

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

Could you check my review?

customized_list_view/models/custom_list.py Outdated Show resolved Hide resolved
def _button_immediate_function(self, function):
res = super(Module, self)._button_immediate_function(function)
views_mods = self.env["custom.list.view"].search([])
for vm in views_mods:

Choose a reason for hiding this comment

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

This solution is not optimal.
Please change this implementation without 2 cycles. If you want remove all 'custom.list.view.line' records without field_id value, you can use search for this. I think it's faster then you solution.

customized_list_view/tests/test_customized_list_view.py Outdated Show resolved Hide resolved
@ilyasProgrammer ilyasProgrammer force-pushed the 14.0-customized_list_view branch 2 times, most recently from ee8a1b8 to 7b0eb28 Compare April 5, 2024 11:00
Copy link

@geomer198 geomer198 left a comment

Choose a reason for hiding this comment

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

Please, add little fix.

active = fields.Boolean(default=True)
name = fields.Char(required=True)
model_id = fields.Many2one("ir.model", required=True, ondelete="cascade")
model_name = fields.Char(related="model_id.model", store=True)

Choose a reason for hiding this comment

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

For what you use store here?

Copy link
Member Author

Choose a reason for hiding this comment

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

To avoid recompute.

cls.users_model = cls.env["ir.model"]._get("res.users")
cls.view_users_tree = cls.env.ref("base.view_users_tree")

def test_all_customized_list_view(self):

Choose a reason for hiding this comment

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

Could you add doc string for this test method, which describe what this method do?

Copy link
Member Author

Choose a reason for hiding this comment

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

The method is for tests. There is comment for each subtest and message for each assert. It is sufficient.

Copy link

@geomer198 geomer198 left a comment

Choose a reason for hiding this comment

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

LGTM!

@francesco-ooops
Copy link
Contributor

@pedrobaeza could you merge this one? thanks!

@pedrobaeza pedrobaeza added this to the 14.0 milestone Apr 19, 2024
@pedrobaeza
Copy link
Member

Being a modification of the web client, it should be prefixed with web_ IMO. A name can be web_tree_customized_field_list.

@francesco-ooops
Copy link
Contributor

@pedrobaeza should it be in this repo or in OCA/web in your opinion?

@pedrobaeza
Copy link
Member

Good question... Let's keep it here as is less crowded.

@ilyasProgrammer ilyasProgrammer changed the title [14.0][ADD] customized_list_view [14.0][ADD] web_tree_customized_field_list Apr 22, 2024
@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

Copy link
Contributor

@francesco-ooops francesco-ooops left a comment

Choose a reason for hiding this comment

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

LGTM

@pedrobaeza shall we go?

@pedrobaeza
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 14.0-ocabot-merge-pr-623-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit e8e0075 into OCA:14.0 May 2, 2024
5 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 7054737. Thanks a lot for contributing to OCA. ❤️

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