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] business_requirement #2

Merged
merged 16 commits into from Jun 22, 2016

Conversation

Projects
None yet
10 participants
@victormmtorres
Copy link
Collaborator

commented Jun 2, 2016

new file: business_requirement/README.rst
new file: business_requirement/init.py
new file: business_requirement/openerp.py
new file: business_requirement/data/business_data.xml
new file: business_requirement/models/init.py
new file: business_requirement/models/business.py
new file: business_requirement/security/ir.model.access.csv
new file: business_requirement/security/security.xml
new file: business_requirement/static/description/icon.png
new file: business_requirement/static/img/bus_req.png
new file: business_requirement/static/img/bus_req_alias.png
new file: business_requirement/static/img/bus_req_approved.png
new file: business_requirement/static/img/bus_req_cancel.png
new file: business_requirement/static/img/bus_req_confirmed.png
new file: business_requirement/static/img/bus_req_cust_story.png
new file: business_requirement/static/img/bus_req_done.png
new file: business_requirement/static/img/bus_req_drop.png
new file: business_requirement/static/img/bus_req_module_diag.png
new file: business_requirement/static/img/bus_req_tags.png
new file: business_requirement/static/img/bus_req_tags2.png
new file: business_requirement/static/img/bus_req_tree.png
new file: business_requirement/static/img/module_diag.graphml
new file: business_requirement/tests/init.py
new file: business_requirement/tests/test_br.py
new file: business_requirement/views/business_view.xml
new file: business_requirement/wizards/init.py
new file: business_requirement/wizards/mail_compose_message.py

def default_get(self, cr, uid, fields, context=None):
res = super(MailComposeMessage, self).default_get(cr, uid,
fields, context=context)
if context.get('default_model') == 'business.requirement' and \
context.get('default_res_id'):
br_rec = self.pool.get(
context.get('default_model')
).browse(cr, uid, context['default_res_id'])
res['subject'] = 'Re: %s-%s' % (br_rec.name, br_rec.description)
return res

   

@victormmtorres

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 2, 2016

This is the new PR which replaces the old one: OCA/project#127

@elicoidal

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2016

👍
@pedrobaeza I cannot add the labels in repo: any idea?

@dreispt

This comment has been minimized.

Copy link
Member

commented Jun 2, 2016

@victormartinelicocorp We should merge #1 and then rebase this. Can you have a look there?

Also, no need to provide a list of the changed files, thatis the diff already.
Instead, provide some insight on what changed. In this case, your second comment is the relevant one.

@victormmtorres

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 2, 2016

@dreispt Understood that. I will do the same for the other PR #3 and wait for the next ones.
Thanks!

@dreispt

This comment has been minimized.

Copy link
Member

commented Jun 2, 2016

#1 merged, @victormartinelicocorp you can fetch and rebase now.

@elicoidal

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2016

Thanks!

victor
[ADD] business_requirement
	new file:   business_requirement/README.rst
	new file:   business_requirement/__init__.py
	new file:   business_requirement/__openerp__.py
	new file:   business_requirement/data/business_data.xml
	new file:   business_requirement/models/__init__.py
	new file:   business_requirement/models/business.py
	new file:   business_requirement/security/ir.model.access.csv
	new file:   business_requirement/security/security.xml
	new file:   business_requirement/static/description/icon.png
	new file:   business_requirement/static/img/bus_req.png
	new file:   business_requirement/static/img/bus_req_alias.png
	new file:   business_requirement/static/img/bus_req_approved.png
	new file:   business_requirement/static/img/bus_req_cancel.png
	new file:   business_requirement/static/img/bus_req_confirmed.png
	new file:   business_requirement/static/img/bus_req_cust_story.png
	new file:   business_requirement/static/img/bus_req_done.png
	new file:   business_requirement/static/img/bus_req_drop.png
	new file:   business_requirement/static/img/bus_req_module_diag.png
	new file:   business_requirement/static/img/bus_req_tags.png
	new file:   business_requirement/static/img/bus_req_tags2.png
	new file:   business_requirement/static/img/bus_req_tree.png
	new file:   business_requirement/static/img/module_diag.graphml
	new file:   business_requirement/tests/__init__.py
	new file:   business_requirement/tests/test_br.py
	new file:   business_requirement/views/business_view.xml
	new file:   business_requirement/wizards/__init__.py
	new file:   business_requirement/wizards/mail_compose_message.py

@victormmtorres victormmtorres force-pushed the victormmtorres:0_business_requirement_base branch from 6653215 to da0ea5b Jun 3, 2016

@victormmtorres

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 3, 2016

@elicoidal @dreispt Done. Thanks!

@coveralls

This comment has been minimized.

Copy link

commented Jun 3, 2016

Coverage Status

Coverage remained the same at 60.104% when pulling da0ea5b on victormartinelicocorp:0_business_requirement_base into af25af1 on OCA:8.0.

If you spotted it first, help us smashing it by providing a detailed and welcomed feedback `here <https://github.com/OCA/
project/issues/new?body=module:%20
business_requirement%0Aversion:%20
8.0%0A%0A**Steps%20to%20reproduce**%0A-%20...%0A%0A**Current%20behavior**%0A%0A**Expected%20behavior**>`_.

This comment has been minimized.

Copy link
@dreispt

dreispt Jun 3, 2016

Member

This might be the line generating the rst error. Remove the line break.
See http://www.silas.net.br/tech/devel/rst.html

This comment has been minimized.

Copy link
@dreispt
Overwrite method message_post from mail.thread to modify the default
behavior of subject with mail messages.
"""
# ---------------------------- changes ends here 2016/03/08

This comment has been minimized.

Copy link
@dreispt

dreispt Jun 3, 2016

Member

Remove comment lines to have a single docstring

This comment has been minimized.

Copy link
@dreispt
victor
[FIX] Comments lines and Rst File
	modified:   business_requirement/README.rst
	modified:   business_requirement/models/business.py
@coveralls

This comment has been minimized.

Copy link

commented Jun 7, 2016

Coverage Status

Coverage remained the same at 60.104% when pulling 96e15f9 on victormartinelicocorp:0_business_requirement_base into af25af1 on OCA:8.0.

@elicoidal

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2016

@dreispt Thanks! Travis is green :)
@pedrobaeza @jbeficent Could we get a review and work on merging this PR.
It has been a long time now and we are eager to propose a first stable version.


The set comprises of multiple modules that can be used independently or not:

=========================================== ====================================

This comment has been minimized.

Copy link
@pedrobaeza

pedrobaeza Jun 7, 2016

Contributor

I think this a description for the README.md of the repository, not this module, and it's even automatically done by the module listing bot.

This comment has been minimized.

Copy link
@elicoidal

elicoidal Jun 8, 2016

Contributor

Thanks for the review :)
I will move that part to the README.md

This comment has been minimized.

Copy link
@dreispt
'Name',
required=False,
readonly=True,
copy=False,

This comment has been minimized.

Copy link
@pedrobaeza

pedrobaeza Jun 7, 2016

Contributor

Why don't you make the name required, put copy=True and overwrite copy method to get the name with for example * at the end?

This comment has been minimized.

Copy link
@elicoidal

elicoidal Jun 8, 2016

Contributor
  • Name is linked to a sequence
  • Description is the title of the BR

So we should have a new sequence generated at copy method if I understand properly

This comment has been minimized.

Copy link
@pedrobaeza

pedrobaeza Jun 8, 2016

Contributor

Why you have then another field called 'sequence'?

This comment has been minimized.

Copy link
@elicoidal

elicoidal Jun 8, 2016

Contributor

"Sequence" fields are for drag and drop and sorting management

This comment has been minimized.

Copy link
@pedrobaeza

pedrobaeza Jun 8, 2016

Contributor

Ah, OK, sorry for the confusion. It's the standard nomenclature...

This comment has been minimized.

Copy link
@elicoidal

elicoidal Jun 10, 2016

Contributor

similar to your suggestion, the idea of a sequence was introduced for later use of Kanban view.

This comment has been minimized.

Copy link
@jbeficent

jbeficent Jun 10, 2016

Member

When you drag and drop a requirement using the handle, a js component will re-assign numbers for all requirements that have been loaded on that domain. That's why it's important to make sure that when you reorder, you do it in the appropiate domain (e.g. project).

Not sure how you'd use the sequence field in the Kanban view.

This comment has been minimized.

Copy link
@elicoidal

elicoidal Jun 10, 2016

Contributor

True. My idea was to have a mix of priority and sequence to be able to drag n drop and follow most important BR per project.
Kanban could be per stage with filtering by project or customer.
All in all, no decision made but in this module I wanted to provide the most standard and basic field for later use.
If no consensus on this usage we might reconsider (we do not use it yet because the Kanban view is not yet implemented)

This comment has been minimized.

Copy link
@victormmtorres

victormmtorres Jun 21, 2016

Author Collaborator

We will remove the sequence from the tree view

This comment has been minimized.

Copy link
@dreispt
copy=False,
states={'draft': [('readonly', False)]}
)
description = fields.Char(

This comment has been minimized.

Copy link
@pedrobaeza

pedrobaeza Jun 7, 2016

Contributor

Why having name and description?

This comment has been minimized.

Copy link
@victormmtorres

victormmtorres Jun 21, 2016

Author Collaborator

answered above

This comment has been minimized.

Copy link
@dreispt
readonly=True,
states={'draft': [('readonly', False)]}
)
business_requirement = fields.Text(

This comment has been minimized.

Copy link
@pedrobaeza

pedrobaeza Jun 7, 2016

Contributor

Put directly fields.Html and you won't need to put the widget in views.

This comment has been minimized.

Copy link
@dreispt
Should only be set by Chatter.
:return int: ID of newly created mail.message
"""
if context.get('default_model') == 'business.requirement':

This comment has been minimized.

Copy link
@pedrobaeza

pedrobaeza Jun 7, 2016

Contributor

This is definitively a 👎

You can't overwrite totally this method only for making a little change. You have to find a way of getting your behavior without copy/pasting the code.

This comment has been minimized.

Copy link
@yostashiro

yostashiro Jun 9, 2016

Member

Would this do?

    def default_get(self, cr, uid, fields, context=None):
        res = super(MailComposeMessage, self).default_get(cr, uid,
            fields, context=context)
        if context.get('default_model') == 'business.requirement' and \
            context.get('default_res_id'):
            br_rec = self.pool.get(
                context.get('default_model')
            ).browse(cr, uid, context['default_res_id'])
            res['subject'] = 'Re: %s-%s' % (br_rec.name, br_rec.description)
        return res

This comment has been minimized.

Copy link
@elicoidal

elicoidal Jun 10, 2016

Contributor

Looks better indeed. @pedrobaeza @victormartinelicocorp

This comment has been minimized.

Copy link
@dreispt
@pedrobaeza

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2016

Crop the icon to get the full size instead of keeping a transparent border.

result = super(MailComposeMessage, self).default_get(
cr, uid, fields, context=context)

# v6.1 compatibility mode

This comment has been minimized.

Copy link
@pedrobaeza

pedrobaeza Jun 7, 2016

Contributor

This shouldn't go here in this module. If you have found a problem in the composition wizard, put a PR/issue against Odoo/OCB or make a specific patch module that deals with it, but don't insert this in a module about "Business requirements"

This comment has been minimized.

Copy link
@elicoidal

elicoidal Jun 8, 2016

Contributor

@pedrobaeza Thanks for the advice: we will recheck the methods about message (@duanyp1991 @seb-elico)

@jbeficent

This comment has been minimized.

Copy link
Member

commented Jun 10, 2016

@victormartinelicocorp @elicoidal When using the module I find it missing a field 'Requested by'.
As I'm conducting interviews in gap analysis sessions with end users during this days, I am the one capturing the requirements, and hence I am the one logged as 'created_by'. But I want to reflect that I am creating a requirement that has been requested by another user.

IMHO the system should default me as 'Requested by', but I should have the possibility to change it to someone else. Also, the requested by should become a follower of the business requirement when he's assigned, so that he's on the loop of the updates.

victor
[IMP] business_requirement: Adapt current workflow.
If confirmed the user can't modify the first tab (story/scenario/gap)
but can still change the second one (Other information).
If approved, the user can't change the first and second tab.
	modified:   business_requirement/models/business.py
@coveralls

This comment has been minimized.

Copy link

commented Jun 20, 2016

Coverage Status

Coverage remained the same at 87.097% when pulling 8c858ee on victormartinelicocorp:0_business_requirement_base into af25af1 on OCA:8.0.

victor
[IMP] business_requirement: Modify business requirement categories
to be simplify the name to "Categories".
Change business requirement "description" to full screen wide.
	modified:   business_requirement/models/business.py
	modified:   business_requirement/views/business_view.xml
@victormmtorres

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 20, 2016

@coveralls

This comment has been minimized.

Copy link

commented Jun 20, 2016

Coverage Status

Coverage remained the same at 87.097% when pulling 4bfb8ab on victormartinelicocorp:0_business_requirement_base into af25af1 on OCA:8.0.

Victor Martin
Merge pull request #9 from elicoidal/0_business_requirement_base
added the module in the image path in README for business_requirement base module.
@coveralls

This comment has been minimized.

Copy link

commented Jun 21, 2016

Coverage Status

Coverage remained the same at 87.097% when pulling cda7844 on victormartinelicocorp:0_business_requirement_base into af25af1 on OCA:8.0.

victor
[IMP] business_requirement: Change Text field by Html to avoid use Ht…
…ml widget on the view.

Remove sequence field on the Tree view. Next version will use sequence for the Kanban view.
	modified:   business_requirement/models/business.py
	modified:   business_requirement/views/business_view.xml
@victormmtorres

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 21, 2016

@elicoidal I modified the comments left. Could you have a check?

@coveralls

This comment has been minimized.

Copy link

commented Jun 21, 2016

Coverage Status

Coverage remained the same at 87.097% when pulling fb120a1 on victormartinelicocorp:0_business_requirement_base into af25af1 on OCA:8.0.

@elicoidal

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2016

👍
@pedrobaeza @dreispt would you be so kind to help us get an extra +1 and merge this PR?

states={'draft': [('readonly', False)]}
)
state = fields.Selection(
selection="_get_states",

This comment has been minimized.

Copy link
@dreispt

dreispt Jun 21, 2016

Member

For future work: use Stages instead of states. The base_stage_state (still a WIP) could be a quick and easy way to do that.

This comment has been minimized.

Copy link
@elicoidal

elicoidal Jun 22, 2016

Contributor

We will change it now (s/state/stage) in order to avoid migration later.

This comment has been minimized.

Copy link
@victormmtorres

This comment has been minimized.

Copy link
@duanyp1991

duanyp1991 Jun 22, 2016

Member

@elicoidal @victormartinelicocorp we can keep it like this since the module is still in WIP. The migration is still needed even if we change the name now. :)

return br.parent_id and br.parent_id.level + 1 or 1

for br in self:
level = _compute_level(br)

This comment has been minimized.

Copy link
@dreispt

dreispt Jun 21, 2016

Member

Nitpick: I don't see the point of isolating this expression in a function.

This comment has been minimized.

Copy link
@victormmtorres

This comment has been minimized.

Copy link
@duanyp1991

duanyp1991 Jun 22, 2016

Member

@victormartinelicocorp agree with dreispt :)


@api.multi
@api.depends('business_requirement_ids')
def _sub_br_count(self):

This comment has been minimized.

Copy link
@dreispt

dreispt Jun 21, 2016

Member

Following the guideline, this should be name _compute_...
I believe this is being enforced on newer builds.

This comment has been minimized.

Copy link
@elicoidal

elicoidal Jun 22, 2016

Contributor

we will update accordingly

Known issues / Roadmap
======================

* Multi-company management

This comment has been minimized.

Copy link
@dreispt

dreispt Jun 21, 2016

Member

Isn't multicompany already implemented here?

This comment has been minimized.

Copy link
@elicoidal

elicoidal Jun 22, 2016

Contributor

Partially: the field exists but there is no complete ACL and it has not been fully tested.
Effort to get there is small though

@dreispt

This comment has been minimized.

Copy link
Member

commented Jun 21, 2016

👍 I pointed out some nitpicks, but nothing worth blocking a merge.

PS: As an experiment, I enabled CodeReviewHub for this repo. Sorry for any noise.

@dreispt

This comment has been minimized.

Copy link
Member

commented Jun 21, 2016

:shipit:

@elicoidal

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2016

@dreispt Thanks for the time to review and your advices! This is really appreciated.
We will fix simple details and merge the PR.

victor
[FIX] business_requirement: Change some model functions name to follo…
…w guideline

and remove not necessary isolated functions.
	modified:   business_requirement/models/business.py
@victormmtorres

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 22, 2016

@coveralls

This comment has been minimized.

Copy link

commented Jun 22, 2016

Coverage Status

Coverage remained the same at 86.813% when pulling ba7a4f4 on victormartinelicocorp:0_business_requirement_base into af25af1 on OCA:8.0.

@dreispt

This comment has been minimized.

Copy link
Member

commented Jun 22, 2016

👍

@elicoidal elicoidal merged commit 42a9397 into OCA:8.0 Jun 22, 2016

3 checks passed

ci/runbot runbot build 3149184-2-ba7a4f (runtime 35s)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 86.813%
Details
@elicoidal

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2016

@dreispt @jbeficent @pedrobaeza Thanks for your help and review!
We will focus now on the deliverable part :)

@jbeficent

This comment has been minimized.

Copy link
Member

commented Jun 22, 2016

@elicoidal Thanks to you and @victormartinelicocorp! It is a great contribution.

@elicoidal elicoidal referenced this pull request Jun 22, 2016

Merged

[ADD] business_requirement_deliverable_categ #4

1 of 2 tasks complete

yvaucher pushed a commit that referenced this pull request Jul 5, 2016

elicoidal added a commit that referenced this pull request Sep 30, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.