Create alternate route for users that cannot force assign.

[Ian Wilson, 2010-01-15 19:27:33]

http://codereview.appspot.com/189092/diff/1/2
File shipment.py (right):

http://codereview.appspot.com/189092/diff/1/2#newcode1269
shipment.py:1269: },
There seemed no way to just hide the button in a dialog so I created an
alternate path to notify users without the ability to force assign that
assigning failed.

http://codereview.appspot.com/189092/diff/1/2#newcode1304
shipment.py:1304: if group.name == 'Stock Force Assignment':
Is there a better way to check if a user belongs to a group specified in
XML(stock.xml) ?

[udono, 2010-01-15 20:08:05]

Ok for me

http://codereview.appspot.com/189092/diff/1/2
File shipment.py (right):

http://codereview.appspot.com/189092/diff/1/2#newcode1269
shipment.py:1269: },
On 2010/01/15 19:27:33, Ian Wilson wrote:
> There seemed no way to just hide the button in a dialog so I created an
> alternate path to notify users without the ability to force assign that
> assigning failed.

I think its the right way you go.

http://codereview.appspot.com/189092/diff/1/2#newcode1304
shipment.py:1304: if group.name == 'Stock Force Assignment':
On 2010/01/15 19:27:33, Ian Wilson wrote:
> Is there a better way to check if a user belongs to a group specified in
> XML(stock.xml) ?

I don't know a better way.

[Ian Wilson, 2010-01-15 20:13:08]

On 2010/01/15 20:08:05, udo.spallek wrote:
> Ok for me
> 
> http://codereview.appspot.com/189092/diff/1/2
> File shipment.py (right):
> 
> http://codereview.appspot.com/189092/diff/1/2#newcode1269
> shipment.py:1269: },
> On 2010/01/15 19:27:33, Ian Wilson wrote:
> > There seemed no way to just hide the button in a dialog so I created an
> > alternate path to notify users without the ability to force assign that
> > assigning failed.
> 
> I think its the right way you go.
> 
> http://codereview.appspot.com/189092/diff/1/2#newcode1304
> shipment.py:1304: if group.name == 'Stock Force Assignment':
> On 2010/01/15 19:27:33, Ian Wilson wrote:
> > Is there a better way to check if a user belongs to a group specified in
> > XML(stock.xml) ?
> 
> I don't know a better way.

Okay I still have to complete it for the other assign wizards as well, I think
return shipment and internal shipment.

[ced, 2010-01-15 23:23:25]

http://codereview.appspot.com/189092/diff/1/2
File shipment.py (right):

http://codereview.appspot.com/189092/diff/1/2#newcode1260
shipment.py:1260: 'notify': {
I think it should be named, assign_failed

http://codereview.appspot.com/189092/diff/1/2#newcode1264
shipment.py:1264: 'object': 'stock.shipment.out.assign.ask_force',
You must use an other object

http://codereview.appspot.com/189092/diff/1/2#newcode1301
shipment.py:1301: users = user_obj.browse(cursor, user, [user], context=context)
It will be better to search on group with users = user and id = group id.

http://codereview.appspot.com/189092/diff/1/2#newcode1304
shipment.py:1304: if group.name == 'Stock Force Assignment':
On 2010/01/15 19:27:33, Ian Wilson wrote:
> Is there a better way to check if a user belongs to a group specified in
> XML(stock.xml) ?

You must use ir.model.data to get the id.
But I think it will be best to read the group id from the workflow.transition.

[Ian Wilson, 2010-01-16 02:39:39]

http://codereview.appspot.com/189092/diff/1/2
File shipment.py (right):

http://codereview.appspot.com/189092/diff/1/2#newcode1304
shipment.py:1304: if group.name == 'Stock Force Assignment':
On 2010/01/15 23:23:25, ced wrote:
> On 2010/01/15 19:27:33, Ian Wilson wrote:
> > Is there a better way to check if a user belongs to a group specified in
> > XML(stock.xml) ?
> 
> You must use ir.model.data to get the id.
> But I think it will be best to read the group id from the workflow.transition.
> 
I think I understand all the comments except for this one.  What do you mean by
"read the group id from the workflow.transition"?  From
shipmentinternal_trans_waiting_assigned_force ?

[ced, 2010-01-16 09:12:33]

http://codereview.appspot.com/189092/diff/1/2
File shipment.py (right):

http://codereview.appspot.com/189092/diff/1/2#newcode1304
shipment.py:1304: if group.name == 'Stock Force Assignment':
On 2010/01/16 02:39:39, Ian Wilson wrote:
> On 2010/01/15 23:23:25, ced wrote:
> > On 2010/01/15 19:27:33, Ian Wilson wrote:
> > > Is there a better way to check if a user belongs to a group specified in
> > > XML(stock.xml) ?
> > 
> > You must use ir.model.data to get the id.
> > But I think it will be best to read the group id from the
workflow.transition.
> > 
> I think I understand all the comments except for this one.  What do you mean
by
> "read the group id from the workflow.transition"?  From
> shipmentinternal_trans_waiting_assigned_force ?
> 

Yes but don't forget that there is also a wizard stock.shipment.internal.assign

[Ian Wilson, 2010-01-16 20:29:39]

This does not seem to work, is this what you mean?

trans_ids = workflow_transition_obj.search(cursor, user, \
   [('id', '=','shipmentout_trans_waiting_assigned_force')],
    context=context)

Ie. how do I get the transition instance with the id from the xml?


On 2010/01/16 09:12:33, ced wrote:
> http://codereview.appspot.com/189092/diff/1/2
> File shipment.py (right):
> 
> http://codereview.appspot.com/189092/diff/1/2#newcode1304
> shipment.py:1304: if group.name == 'Stock Force Assignment':
> On 2010/01/16 02:39:39, Ian Wilson wrote:
> > On 2010/01/15 23:23:25, ced wrote:
> > > On 2010/01/15 19:27:33, Ian Wilson wrote:
> > > > Is there a better way to check if a user belongs to a group specified in
> > > > XML(stock.xml) ?
> > > 
> > > You must use ir.model.data to get the id.
> > > But I think it will be best to read the group id from the
> workflow.transition.
> > > 
> > I think I understand all the comments except for this one.  What do you mean
> by
> > "read the group id from the workflow.transition"?  From
> > shipmentinternal_trans_waiting_assigned_force ?
> > 
> 
> Yes but don't forget that there is also a wizard
stock.shipment.internal.assign

[ced, 2010-01-16 20:43:09]

On 2010/01/16 20:29:39, Ian Wilson wrote:
> This does not seem to work, is this what you mean?
> 
> trans_ids = workflow_transition_obj.search(cursor, user, \
>    [('id', '=','shipmentout_trans_waiting_assigned_force')],
>     context=context)
> 
> Ie. how do I get the transition instance with the id from the xml?
> 

Look at
http://hg.tryton.org/hgwebdir.cgi/modules/account/file/dc6441bc6fee/account.p...

[Ian Wilson, 2010-01-19 21:36:30]

Added models for customer return and internal shipments.  Also used distinct
objects for assign_failed in the workflows.

[ced, 2010-01-19 22:46:50]

http://codereview.appspot.com/189092/diff/1006/2001
File shipment.py (right):

http://codereview.appspot.com/189092/diff/1006/2001#newcode1239
shipment.py:1239: class AssignShipmentOutAssignFailed(ModelView):
Same class than AssignShipmentOutAskForce, you could inherit from it.

http://codereview.appspot.com/189092/diff/1006/2001#newcode1275
shipment.py:1275: 'object': 'stock.shipment.out.assign.assign_failed',
You could use here stock.shipment.out.assign.ask_force
and perhaps rename it.

http://codereview.appspot.com/189092/diff/1006/2001#newcode1323
shipment.py:1323: context=context)
We indent with 8 spaces.

http://codereview.appspot.com/189092/diff/1006/2001#newcode1580
shipment.py:1580: ('fs_id', '=',
'shipmentinternal_trans_waiting_assigned_force'),
80 cols.

http://codereview.appspot.com/189092/diff/1006/2002
File shipment.xml (right):

http://codereview.appspot.com/189092/diff/1006/2002#newcode261
shipment.xml:261: <record model="ir.ui.view"
id="shipment_in_return_assign_assign_failed_view_form">
It is the same form than shipment_in_return_assign_ask_force_view_form, you
could inherit from it.

[Ian Wilson, 2010-01-19 23:12:48]

I commented on your first few comments and I understand the spacing and length
comments and will fix that in next upload.

http://codereview.appspot.com/189092/diff/1006/2001
File shipment.py (right):

http://codereview.appspot.com/189092/diff/1006/2001#newcode1239
shipment.py:1239: class AssignShipmentOutAssignFailed(ModelView):
On 2010/01/19 22:46:50, ced wrote:
> Same class than AssignShipmentOutAskForce, you could inherit from it.

It doesn't seem to make sense for either class to inherit from the other. Does
the assign failed model really extend the force assign model?    I was going to
just share the same class between assign_failed and force_assign but you
suggested "You must use an other object".  You meant making an additional class
right?

http://codereview.appspot.com/189092/diff/1006/2001#newcode1275
shipment.py:1275: 'object': 'stock.shipment.out.assign.assign_failed',
On 2010/01/19 22:46:50, ced wrote:
> You could use here stock.shipment.out.assign.ask_force
> and perhaps rename it.

What do you mean rename it?

[ced, 2010-01-19 23:19:30]

On 2010/01/19 23:12:48, Ian Wilson wrote:
> I commented on your first few comments and I understand the spacing and length
> comments and will fix that in next upload.
> 
> http://codereview.appspot.com/189092/diff/1006/2001
> File shipment.py (right):
> 
> http://codereview.appspot.com/189092/diff/1006/2001#newcode1239
> shipment.py:1239: class AssignShipmentOutAssignFailed(ModelView):
> On 2010/01/19 22:46:50, ced wrote:
> > Same class than AssignShipmentOutAskForce, you could inherit from it.
> 
> It doesn't seem to make sense for either class to inherit from the other. Does
> the assign failed model really extend the force assign model?    I was going
to
> just share the same class between assign_failed and force_assign but you
> suggested "You must use an other object".  You meant making an additional
class
> right?
> 
> http://codereview.appspot.com/189092/diff/1006/2001#newcode1275
> shipment.py:1275: 'object': 'stock.shipment.out.assign.assign_failed',
> On 2010/01/19 22:46:50, ced wrote:
> > You could use here stock.shipment.out.assign.ask_force
> > and perhaps rename it.
> 
> What do you mean rename it?

I mean that we could use the same model for the both states. The difference is
simply the buttons.
So we can rename "stock.shipment.out.assign.ask_force" with a better name that
will be good for both cases. This will remove duplicated code.

[Ian Wilson, 2010-01-19 23:49:30]

On 2010/01/19 23:19:30, ced wrote:
> On 2010/01/19 23:12:48, Ian Wilson wrote:
> > I commented on your first few comments and I understand the spacing and
length
> > comments and will fix that in next upload.
> > 
> > http://codereview.appspot.com/189092/diff/1006/2001
> > File shipment.py (right):
> > 
> > http://codereview.appspot.com/189092/diff/1006/2001#newcode1239
> > shipment.py:1239: class AssignShipmentOutAssignFailed(ModelView):
> > On 2010/01/19 22:46:50, ced wrote:
> > > Same class than AssignShipmentOutAskForce, you could inherit from it.
> > 
> > It doesn't seem to make sense for either class to inherit from the other.
Does
> > the assign failed model really extend the force assign model?    I was going
> to
> > just share the same class between assign_failed and force_assign but you
> > suggested "You must use an other object".  You meant making an additional
> class
> > right?
> > 
> > http://codereview.appspot.com/189092/diff/1006/2001#newcode1275
> > shipment.py:1275: 'object': 'stock.shipment.out.assign.assign_failed',
> > On 2010/01/19 22:46:50, ced wrote:
> > > You could use here stock.shipment.out.assign.ask_force
> > > and perhaps rename it.
> > 
> > What do you mean rename it?
> 
> I mean that we could use the same model for the both states. The difference is
> simply the buttons.
> So we can rename "stock.shipment.out.assign.ask_force" with a better name that
> will be good for both cases. This will remove duplicated code.

I think the name "assign failed" for the model is suitable for both cases(one
with the force assign button and one without).
Do I have to do anything special to maintain backward compatibility or can I
just rename the view in the code?  Do I need to inherit from the original view
even though it won't be used for anything?  Sorry I'm not picking this up
faster, everytime you say inherit do you really mean inherit programmatically,
ie. <field name="inherit" ref="shipment_in_return_assign_ask_force_view_form"/>
?

[ced, 2010-01-20 08:07:26]

On 2010/01/19 23:49:30, Ian Wilson wrote:
> On 2010/01/19 23:19:30, ced wrote:
> > On 2010/01/19 23:12:48, Ian Wilson wrote:
> > > I commented on your first few comments and I understand the spacing and
> length
> > > comments and will fix that in next upload.
> > > 
> > > http://codereview.appspot.com/189092/diff/1006/2001
> > > File shipment.py (right):
> > > 
> > > http://codereview.appspot.com/189092/diff/1006/2001#newcode1239
> > > shipment.py:1239: class AssignShipmentOutAssignFailed(ModelView):
> > > On 2010/01/19 22:46:50, ced wrote:
> > > > Same class than AssignShipmentOutAskForce, you could inherit from it.
> > > 
> > > It doesn't seem to make sense for either class to inherit from the other.
> Does
> > > the assign failed model really extend the force assign model?    I was
going
> > to
> > > just share the same class between assign_failed and force_assign but you
> > > suggested "You must use an other object".  You meant making an additional
> > class
> > > right?
> > > 
> > > http://codereview.appspot.com/189092/diff/1006/2001#newcode1275
> > > shipment.py:1275: 'object': 'stock.shipment.out.assign.assign_failed',
> > > On 2010/01/19 22:46:50, ced wrote:
> > > > You could use here stock.shipment.out.assign.ask_force
> > > > and perhaps rename it.
> > > 
> > > What do you mean rename it?
> > 
> > I mean that we could use the same model for the both states. The difference
is
> > simply the buttons.
> > So we can rename "stock.shipment.out.assign.ask_force" with a better name
that
> > will be good for both cases. This will remove duplicated code.
> 
> I think the name "assign failed" for the model is suitable for both cases(one
> with the force assign button and one without).
> Do I have to do anything special to maintain backward compatibility or can I
> just rename the view in the code?

You can also fix the translation files.

> Do I need to inherit from the original view
> even though it won't be used for anything?

No, if we go with the same ModelView.

> Sorry I'm not picking this up
> faster, everytime you say inherit do you really mean inherit programmatically,
> ie. <field name="inherit"
ref="shipment_in_return_assign_ask_force_view_form"/>
> ?

No more needed.

[ced, 2010-01-21 12:13:30]

http://codereview.appspot.com/189092/diff/1010/2007
File shipment.py (right):

http://codereview.appspot.com/189092/diff/1010/2007#newcode1574
shipment.py:1574: model_data_ids = model_data_obj.search(cursor, user, [
Now we use ir.model.data get_id
http://hg.tryton.org/hgwebdir.cgi/trytond/rev/b5ae9f9c48ca

[Ian Wilson, 2010-01-21 21:42:05]

I merged with latest stock module.  Please take a look at translation files to
see if changes were correct.  I added an entry for the okay button displayed
when the user does not have permision to force assign.

[ced, 2010-01-21 23:25:06]

Otherwise looks good.

http://codereview.appspot.com/189092/diff/2019/1020
File de_DE.csv (right):

http://codereview.appspot.com/189092/diff/2019/1020#newcode474
de_DE.csv:474:
wizard_button,"stock.shipment.in.return.assign,assign_failed,end",0,Ok,OK,0
It doesn't respect alphabetical order.

[Ian Wilson, 2010-01-22 00:38:41]

So the entries in translation files are sorted by each column ?  I hope this is
what you meant.

[ced, 2010-01-22 08:48:34]

ok