Skip to content

Conversation

@lbarry-apsl
Copy link
Member

Copy link
Contributor

@mpascuall mpascuall left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

@peluko00 peluko00 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

@BernatObrador BernatObrador left a comment

Choose a reason for hiding this comment

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

This isn't working correctly.

Example from version 16:
image

Current behavior:
image

The ID is no longer inside brackets [ ], now the name is inside instead. And the name it's between ' '. Something seems to be wrong with this.

Also, when you create a new task, it ends up like this:
image

@lbarry-apsl lbarry-apsl force-pushed the 17.0-mig-project_task_name_with_id branch from 10ed5ed to ab2103e Compare April 14, 2025 09:09
@lbarry-apsl lbarry-apsl force-pushed the 17.0-mig-project_task_name_with_id branch from ab2103e to 3387508 Compare April 25, 2025 10:50
@lbarry-apsl
Copy link
Member Author

This isn't working correctly.

Example from version 16: image

Current behavior: image

The ID is no longer inside brackets [ ], now the name is inside instead. And the name it's between ' '. Something seems to be wrong with this.

Also, when you create a new task, it ends up like this: image

Done @BernatObrador !!

Copy link

@BernatObrador BernatObrador left a comment

Choose a reason for hiding this comment

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

LGTM!



class TestProjectTaskID(TransactionCase):
def setUp(self):

Choose a reason for hiding this comment

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

Just a minor comment, test should be using @classsmethod

Suggested change
def setUp(self):
@classmethod
def setUpClass(cls):

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

Comment on lines +15 to +16
if hasattr(task, "key") and task.key:
parts.append(task.key)
Copy link
Member

Choose a reason for hiding this comment

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

Wasn't sure about the validity of this part. Shouldn't we add super()._compute_display_name() at the beginning to adjust the updated display_name, instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

@yostashiro this is to make the module compatible with project_key without depending on it. Or do you think it's not necessary?

Copy link
Member

Choose a reason for hiding this comment

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

@lbarry-apsl IMO, we shouldn't add special logic just to be "compatible" with another module. Would there be any issue if we simply did the following?

    @api.depends("name")
    def _compute_display_name(self):
        super()._compute_display_name()
        for task in self:
            task.display_name = f"[{task.id}] {task.display_name}"

Copy link
Member Author

Choose a reason for hiding this comment

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

There wasn't an error, just that the task.key wasn't displayed in the ticket name added by the project_key module.

@yostashiro if it's not a problem, I can leave it as you've told me.

Copy link
Member Author

Choose a reason for hiding this comment

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

@lbarry-apsl IMO, we shouldn't add special logic just to be "compatible" with another module. Would there be any issue if we simply did the following?

    @api.depends("name")
    def _compute_display_name(self):
        super()._compute_display_name()
        for task in self:
            task.display_name = f"[{task.id}] {task.display_name}"

@yostashiro I have done the tests with what you propose here, but the result with the project_key is this and it is not correct [5] 5['OD-1', 'Energy Certificate']

Copy link
Member

Choose a reason for hiding this comment

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

I have done the tests with what you propose here, but the result with the project_key is this and it is not correct [5] 5['OD-1', 'Energy Certificate'].

@lbarry-apsl I think that's because how project_key doesn't use super() in _compute_display_name(). Likewise, the current implementation of this module may break the presentation of the display name if there is another module which manipulates with the field.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I understand. I mentioned in this PR #1503 that it adds the super() function without modifying the logic.

Copy link
Contributor

@AungKoKoLin1997 AungKoKoLin1997 May 14, 2025

Choose a reason for hiding this comment

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

@lbarry-apsl @yostashiro I created PRs to avoid complete overidding.
#1502
#1503

{
"name": "Project Task Name with ID",
"category": "Project",
"version": "17.0.1.0.1",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"version": "17.0.1.0.1",
"version": "17.0.1.0.0",

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

@lbarry-apsl lbarry-apsl force-pushed the 17.0-mig-project_task_name_with_id branch from 3387508 to e885105 Compare May 5, 2025 06:30
@lbarry-apsl
Copy link
Member Author

Hi @pedrobaeza , can you check this migration?

@pedrobaeza
Copy link
Member

/ocabot migration project_task_name_with_id
/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 17.0-ocabot-merge-pr-1472-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot added this to the 17.0 milestone May 13, 2025
@OCA-git-bot OCA-git-bot mentioned this pull request May 13, 2025
37 tasks
@OCA-git-bot OCA-git-bot merged commit ae5f3de into OCA:17.0 May 13, 2025
5 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 1c65bca. 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.

9 participants