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][BR1228][T24770]Extra time application #206

Closed
wants to merge 16 commits into from

Conversation

XXXXLM
Copy link

@XXXXLM XXXXLM commented Dec 14, 2017

No description provided.

@elicoidal
Copy link
Contributor

Is it ready to be reviewed @Miya609 ?

@XXXXLM
Copy link
Author

XXXXLM commented Dec 15, 2017 via email

Copy link

@lonelysun lonelysun left a comment

Choose a reason for hiding this comment

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

please fix


{
'name': 'Extra Time Application',
'version': '10.0.1.0.0',

Choose a reason for hiding this comment

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

why version is '10.0.1.0.0'?

user = self.env.user
is_exist = user.has_group(
'extra_time_application.group_project_task_manager')
remaining = self.remaining_hours

Choose a reason for hiding this comment

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

adapt api.multi

'The task have no enough time, '
'please Apply for more extra time'
))
if not vals.get('project_id.is_modified'):

Choose a reason for hiding this comment

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

what't this?

submit_user_id = fields.Many2one(
'res.users', 'Applicant', help='Applicant',
)
task_no = fields.Many2one('project.task', 'Task No', help='Task No')

Choose a reason for hiding this comment

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

m2o field should be named xxx_id

Copy link
Contributor

@elicoidal elicoidal left a comment

Choose a reason for hiding this comment

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

Too many simple mistakes

@@ -0,0 +1,11 @@
Elico Proprietary License v1.0
Copy link
Contributor

Choose a reason for hiding this comment

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

No need this file. This module can be AGPLv3

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

* Miya Xing <[email protected]>

Copy link
Contributor

Choose a reason for hiding this comment

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

add the other contributors (Eric, Sebastien, Hulk)


{
'name': 'Extra Time Application',
'version': '10.0.0.0.0',
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually @lonelysun 10.0.1.0.0 should be the correct start (first version of the module in v10)

'version': '10.0.0.0.0',
'author': "Elico Corp",
'website': 'https://www.elico-corp.com',
'license': 'Other proprietary',
Copy link
Contributor

Choose a reason for hiding this comment

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

You are not being logical/consistent: here it says proprietary but everywhere you say AGPL!
license should be AGPLv3

log.env['extra.time.application'].create({
'submit_user_id': user.id,
'task_id': log.id,
'reason': 'Automaticity create From PM or Reviewer',
Copy link
Contributor

Choose a reason for hiding this comment

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

Automatically created from PM or Reviewer

@@ -0,0 +1,29 @@
# -*- coding: utf-8 -*-
# © 2017 Elico Corp (www.elico-corp.com)
# Elico Proprietary License v1.0.
Copy link
Contributor

Choose a reason for hiding this comment

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

license is AGPL (everywhere)

self.task_1.write(vals_2)
try:
self.task_2.write(vals_2)
except Exception:
Copy link
Contributor

Choose a reason for hiding this comment

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

I am surprised: should not this trigger an error so that the test fails?

self.sub_extra_time = 0
self.timesheet_ids = self.env['account.analytic.line'].write([[
0, False, {
'date_time': '2017-12-15',
Copy link
Contributor

Choose a reason for hiding this comment

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

For all date, I would suggest to use today() and relative dates in the future today()+2 for example

@api.multi
def subscribe(self):
user = self.env.user
is_exist = user.has_group(
Copy link
Contributor

Choose a reason for hiding this comment

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

Improve the variable name

self.env['extra.time.application'].create({
'submit_user_id': self.submit_user_id.id,
'task_id': self.task_id.id,
'reason': 'Automaticity create From PM or Reviewer',
Copy link
Contributor

Choose a reason for hiding this comment

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

Automatically created from

@XXXXLM
Copy link
Author

XXXXLM commented Dec 19, 2017

@elicoidal No need for draft state,and I fix other error,could you review it?

@elicoidal
Copy link
Contributor

Travis still failing it seems

class ProjectProjectInherit(models.Model):
_inherit = 'project.project'

is_modified = fields.Boolean()
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this for?
This column is never assigned and has no title.
What is the usage?
cc @lonelysun

@XXXXLM
Copy link
Author

XXXXLM commented Dec 20, 2017

@elicoidal Please review it.

@elicoidal
Copy link
Contributor

Travis is still failing
@Miya609 @lonelysun

@lonelysun
Copy link

lonelysun commented Dec 20, 2017 via email

@XXXXLM
Copy link
Author

XXXXLM commented Jan 12, 2018

@seb-elico Could you tell me the reason that why the travis not working?

@lonelysun
Copy link

move to #210

@lonelysun lonelysun closed this Mar 21, 2018
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.

3 participants