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

[8.0] Port HR Department Sequence #123

Merged
merged 12 commits into from
Nov 12, 2015

Conversation

adrienpeiffer
Copy link
Contributor

Supersedes #119

@ghost
Copy link

ghost commented Jul 28, 2015

👍 if travis is green

@max3903 max3903 mentioned this pull request Jul 28, 2015
58 tasks
@mmalorni
Copy link

Works but the sequence doesn't take into consideration the hierarchy. In fact, every time you play with the sequence in the department form tree view, it screws the department tree view because it adds duplicate sequence numbers. 👍
For exemple: You have sales and marketing, you position sales after marketing (before that, the sequence is 0 for all), you go in the sales department and add a Direct Sales and eCommerce Sales, you move eCommerce before Direct Sales, when you go back to the Department View, the sequence is

  • 1 - Marketing
  • 1 - Elearning Sales
  • 2 - Direct Sales
  • 2 - Sales

Instead of

  • 1 - Marketing
  • 2 - Sales
  • 3 - Elearning Sales
  • 4 - Direct Sales

@dreispt
Copy link
Sponsor Member

dreispt commented Jul 28, 2015

Please, it's best to force-push the branch of the initial PR instead of creating a new one.
This disconsiders the history and effort put in the review of the original PR.

@adrienpeiffer
Copy link
Contributor Author

@dreispt It is what I did. But the PR was closed when I did that

@dreispt
Copy link
Sponsor Member

dreispt commented Jul 29, 2015

@adrienpeiffer Hmm, you're right. Now that you mention that, I got a similar problem after force pushing a PR.
Is that a new GitHub behaviour for PRs?

@adrienpeiffer
Copy link
Contributor Author

I don't really know. I think that what happens when the PR is closed ...

if departments.ids:
return departments.name_get()
return super(HrDepartment, self)\
.name_search(name=name, args=args, operator=operator, limit=limit)
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I think this name_get and name_search should be optional or, even better, configurable.
But that can be a future improvement.

@dreispt
Copy link
Sponsor Member

dreispt commented Jul 31, 2015

👍

@max3903 max3903 added this to the 8.0 milestone Jul 31, 2015
======================

This module was written to allow you to order department by Parent-Child Relationship and by Sequence Number.
It also adds some fields on the department like code and active.
Copy link
Member

Choose a reason for hiding this comment

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

I don't really like the "adds some fields ... like" I prefer to know what I'm installing.

I understand that all fields but those two are used for the parent-child relation. So you could simply remove that "like" and extend a bit with a "It also extends department by adding the two extra fields code and active to ease management with many departments"

@yvaucher
Copy link
Member

I added few remarks

@adrienpeiffer
Copy link
Contributor Author

Thanks @yvaucher. It's done !

@yvaucher
Copy link
Member

Thanks for the fixes 👍

@adrienpeiffer adrienpeiffer force-pushed the 8.0-port-hr-department-sequence-ape branch from fce7f88 to 251fa66 Compare November 9, 2015 16:19
@feketemihai
Copy link
Member

👍 I never knew untill now the widget=handle...Great.

But i think we should change the pr to the department repo. It's more appropriate.

@lmignon
Copy link
Sponsor

lmignon commented Nov 12, 2015

👍 Travis errors seem unrelated to your changes

dreispt added a commit that referenced this pull request Nov 12, 2015
@dreispt dreispt merged commit ed3140f into OCA:8.0 Nov 12, 2015
@adrienpeiffer adrienpeiffer deleted the 8.0-port-hr-department-sequence-ape branch December 11, 2015 14:48
sambarros pushed a commit to sambarros/hr that referenced this pull request Jul 26, 2018
Mraimou pushed a commit to camptocamp/hr that referenced this pull request Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants