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

[10.0][FIX] base_ubl: PartyName node is not mandatory in xsd so it should not be mandatory in code either. #166

Merged
merged 1 commit into from May 13, 2020

Conversation

SimoRubi
Copy link
Member

I have edited one of the test files to allow testing this change, please advise me if I should test it in any other way.

@SimoRubi SimoRubi requested a review from astirpe January 16, 2020 11:50
@astirpe
Copy link
Member

astirpe commented Jan 16, 2020

@SimoRubi I would prefer to keep that test unchanged.
Would be nice if you can create a new extra test for this specific case and perhaps checking that also other non-mandatory fields are correctly handled.
Anyway your PR looks good to me.

@SimoRubi SimoRubi force-pushed the 10.0-fix-base_ubl-name_not_mandatory branch from b12c2f2 to 4c44b38 Compare January 16, 2020 14:30
@SimoRubi
Copy link
Member Author

Hi, thanks for the review!
I have created a separated file for tests as you suggested, and noticed this untested flow:

if existing_quotations:
default_sale_id = False
if len(existing_quotations) == 1:
default_sale_id = existing_quotations[0].id
self.write({
'commercial_partner_id': commercial_partner.id,
'partner_shipping_id': partner_shipping_id,
'state': 'update',
'sale_id': default_sale_id,
'doc_type': parsed_order.get('doc_type'),
})
action = self.env['ir.actions.act_window'].for_xml_id(
'sale_order_import', 'sale_order_import_action')
action['res_id'] = self.id
return action

So I have refactored a little the test class to allow testing this flow and ease adding future new tests.

…ot be mandatory in code either.

Refactoring tests to increase test coverage
@SimoRubi SimoRubi force-pushed the 10.0-fix-base_ubl-name_not_mandatory branch from 4c44b38 to 877a05f Compare January 16, 2020 14:39
@SimoRubi
Copy link
Member Author

@OCA/edi-maintainers can someone take a look? Thanks

Copy link
Member

@astirpe astirpe left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks!

@SimoRubi
Copy link
Member Author

@OCA/edi-maintainers can this be merged or should I change something? Thanks

Copy link
Member

@tafaRU tafaRU left a comment

Choose a reason for hiding this comment

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

LGTM Thanks!

Copy link

@SimoneVagile SimoneVagile left a comment

Choose a reason for hiding this comment

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

Good for me, thanks!

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@bealdav
Copy link
Member

bealdav commented Apr 2, 2020

Please @astirpe maybe you can merge ?

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.

None yet

7 participants