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

feat: new DocTypes "Code List" and "Common Code" #43425

Open
wants to merge 24 commits into
base: develop
Choose a base branch
from

Conversation

barredterra
Copy link
Collaborator

@barredterra barredterra commented Sep 29, 2024

@barredterra
Copy link
Collaborator Author

barredterra commented Sep 30, 2024

  • It is not obvious (and was not intended by me) that the CanonicalVersionUri now has to be used as the name of the code list (for the import to work). E.g. I would not expect the CanonicalVersionUri of the code list linked above to be referenced in most eInvoices. So a more generic name would be useful for retrieving the necessary code list. In the screenshot above, I used the Identification/ShortName.
  • "Additional Data" from the code list is not yet imported (e.g. descriptions, as available in the code list linked above).

Ensure the same reference is not used on a different Common Code of the same Code List.
@blaggacao
Copy link
Collaborator

More exploration from my side: barredterra#1 (Edi Template & Edi Log, squashed your naming fix
there, @barredterra )

@blaggacao
Copy link
Collaborator

blaggacao commented Sep 30, 2024

It is not obvious (and was not intended by me) that the CanonicalVersionUri now has to be used as the name of the code list (for the import to work). E.g. I would not expect the CanonicalVersionUri of the code list linked above to be referenced in most eInvoices.

Firstly, most of the time, however, unlike the common codes, these lists are akin to real constants in an e-invoice template. I would typically hard-code them for each distinct edi template. Here is an example:

XML Example
        <cbc:IdentificationCode
          listAgencyID="6"
          listAgencyName="United Nations Economic Commission for Europe"
          listSchemeURI="urn:oasis:names:specification:ubl:codelist:gc:CountryIdentificationCode-2.1"
        >CO</cbc:IdentificationCode>

Secondly, the title is still user friendly, for display purposes.

Screenshot

image

Thirdly, as per the Genericode XML Schema, CanonicalVersionUri is the primary key of a list, so we would have to go out of our way to work around that. https://docs.oasis-open.org/codelist/genericode/v1.0/os/xsd/genericode.xsd

XML Reference
<xsd:complexType name="Identification">
<xsd:annotation>
<xsd:documentation>Identification and location information (metadata).</xsd:documentation>
</xsd:annotation>
<xsd:sequence>
<xsd:group ref="gc:NameSet">
...
</xsd:group>
<xsd:element name="Version" type="xsd:token">
...
</xsd:element>
<xsd:element name="CanonicalUri" type="xsd:anyURI">
<xsd:annotation>
<xsd:documentation>Canonical URI which uniquely identifies all versions (collectively).</xsd:documentation>
<xsd:documentation>
<rule:text id="R25" category="document">Must be an absolute URI, must not be relative.</rule:text>
</xsd:documentation>
<xsd:documentation>
<rule:text id="R26" category="application">Must not be used as a de facto location URI.</rule:text>
</xsd:documentation>
</xsd:annotation>
</xsd:element>
<xsd:group ref="gc:VersionLocationUriSet">
...
</xsd:group>
<xsd:element name="Agency" minOccurs="0" type="gc:Agency">
...
</xsd:element>
</xsd:sequence>
</xsd:complexType>

...

<xsd:group name="VersionLocationUriSet">
<xsd:annotation>
<xsd:documentation>Identification and location URIs for a version.</xsd:documentation>
</xsd:annotation>
<xsd:sequence>
<xsd:element name="CanonicalVersionUri" type="xsd:anyURI">
...
</xsd:element>
<xsd:element name="LocationUri" minOccurs="0" maxOccurs="unbounded" type="xsd:anyURI">
...
</xsd:element>
<xsd:element name="AlternateFormatLocationUri" minOccurs="0" maxOccurs="unbounded" type="gc:MimeTypedUri">
...
</xsd:element>
</xsd:sequence>
</xsd:group>

Note, that CanonicalUri will be shared between lists, and within VerstionLocationUriSet (the unique identifying set), CanoncialVersionUri is the only mandatory one.

Edit:

I think this is what you where looking for:

image

blaggacao
blaggacao previously approved these changes Sep 30, 2024
Copy link
Collaborator

@blaggacao blaggacao left a comment

Choose a reason for hiding this comment

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

I have contributed the Genericode Importer which triggered a certain amount of polishing and I think this version is ready to merge, now.

We should keep in mind, that we're actively developing the EDI functionality for EU & Colombia in an iterative approach against develop - so more to come, soon.

@blaggacao blaggacao marked this pull request as ready for review September 30, 2024 16:36
@barredterra
Copy link
Collaborator Author

barredterra commented Sep 30, 2024

If we have to use this very technical name, we should at least offer the import from the list view (or from an unsaved form) instead of from the saved form view. It would be kind of weird if the user first needs to manually extract some values from the XML, in order to then import the rest of it.

My approach was originally targeted at, e.g., being able to tell the users to create a Code List called "UNTDID.5305" and make sure it contains the tax codes they care about. The alternative, equally good or even better, would be being able to tell them to just import this code list. Telling them to extract the CanonicalVersionUri does not seem realistic, however.

@blaggacao
Copy link
Collaborator

blaggacao commented Oct 1, 2024

[ ... ] we should at least offer the import from the list view (or from an unsaved form) instead of from the saved form view. It would be kind of weird if the user first needs to manually extract some values from the XML, in order to then import the rest of it.

Telling them to extract the CanonicalVersionUri does not seem realistic, however.

Indeed! — I think this concern is already gracefully implemented?

General Scenario: Add more codes from the same list (e.g. different filters) — form import

Specific Scenario: I accidentially selected the wrong code list for upload

image


General Scenario: Add a new code list and some codes — list import

Implemented on the list views of Code List and Common Code

Specific Scenario: Step Two (immediately after upload); Selection of Import filters

image
Note: no extraction of CanonicalVersionUri by the user; filterable are fields with cardinality <= 5

@blaggacao
Copy link
Collaborator

d540d03

image

@blaggacao
Copy link
Collaborator

We may need to relax the unique condition on code for Common Code. This is a valid Code List:

      <Row>
         <Value ColumnRef="code">
            <SimpleValue>1.50</SimpleValue>
         </Value>
         <Value ColumnRef="name">
            <SimpleValue>ReteFuente</SimpleValue>
         </Value>
         <Value ColumnRef="description">
            <SimpleValue>Compras con tarjeta débito o crédito</SimpleValue>
         </Value>
      </Row>
      <Row>
         <Value ColumnRef="code">
            <SimpleValue>1.50</SimpleValue>
         </Value>
         <Value ColumnRef="name">
            <SimpleValue>ReteFuente</SimpleValue>
         </Value>
         <Value ColumnRef="description">
            <SimpleValue>Compras de bienes o productos agrícolas o pecuarios sin procesamiento industrial</SimpleValue>
         </Value>
      </Row>

@blaggacao blaggacao force-pushed the code-lists branch 2 times, most recently from 4f3cc07 to b751b44 Compare October 1, 2024 11:19
@blaggacao
Copy link
Collaborator

Problem addressed, also in the importer:

image

image

@blaggacao
Copy link
Collaborator

52dcb2b addresses the usability concern even further:

image

@blaggacao
Copy link
Collaborator

blaggacao commented Oct 1, 2024

Testing Resume

  • 23 Code Lists, 27 XML files
  • Faulty XML: ok
  • Undeclared Columns: ok
  • Non unique common_code: ok (eg tax rates) -> added description
  • Guard against accidential structured data manipulation
  • Partial loading of a list
  • Later completion of a list - USER ATTENTION! Duplicates are legal in the data structure, double loading must be avoided with care (no other way)
  • Loading values from different files into the same code list
Screenshot

image

@barredterra
Copy link
Collaborator Author

barredterra commented Oct 1, 2024

Indeed! — I think this concern is already gracefully implemented?

Ah, my bad, I've overlooked this.


I think, from a developer perspective, it would be perfect if we can:

(1) Given a ref_doctype, ref_docname and code list, retrieve the applicable code.
(2) Given a code and code list, retrieve the applicable ref_doctype and ref_docname.

Do you think this unambiguous mapping is feasible?

I'm thinking of adding a util and validation for the second case.


Selecting "URL" on file upload fails. Would be nice if we can support this as well.


@blaggacao
Copy link
Collaborator

blaggacao commented Oct 1, 2024

(2) Given a code and code list, retrieve the applicable ref_doctype and ref_docname.

Given ...
	def validate_distinct_references(self):
		"""Ensure no two Common Codes of the same Code List are linked to the same document."""
		for link in self.applies_to:
			existing_links = frappe.get_all(
				"Common Code",
				filters=[
					["name", "!=", self.name],
					["code_list", "=", self.code_list],
					["Dynamic Link", "link_doctype", "=", link.link_doctype],
					["Dynamic Link", "link_name", "=", link.link_name],
				],
				fields=["name", "common_code"],
			)

			if existing_links:
				existing_link = existing_links[0]
				frappe.throw(
					_("{0} {1} is already linked to Common Code {2}.").format(
						link.link_doctype,
						link.link_name,
						get_link_to_form("Common Code", existing_link["name"], existing_link["common_code"]),
					)
				)

I think we can restrict certain Code Lists (optionally) to a certain doctype and thereby meet the full condition necessary to implement such a util for such code lists.

But you're absolutely right: on the parsing end of the flow, this would quite definitely improve DX.

@blaggacao
Copy link
Collaborator

blaggacao commented Oct 1, 2024

Selecting "URL" on file upload fails. Would be nice if we can support this as well.

94bd743

Also library links (for example to post-load more codes from an already partially imported list)

Depends on: frappe/frappe#27954

@blaggacao
Copy link
Collaborator

blaggacao commented Oct 1, 2024

We probably should also return additional_data from the get_code_for because more often than not, this is actually relevant data when rendering a e-invoice template. I'll check this and confirm during dogfooding.


Wile dogfooding I realise:

As user, I want an easy way to associate all Code List documents with my current document, which are configured to be associatable with that document type (by means of a Code List - Doctype association). Especially for documents which are still metadata, but fall within the life-cycle management of non-power users, such as Suppliers, Customers & Items.


We also should probably implement a stringer (__str__) on CommonCode which automatically renders it's common_code into a jinja template without need for more verbosity.


get_code_for should probably throw a user facing error if no association is found but was expected, indicating that xyz association is missing and needs to be set up.

    # should throw if tax_category_id was not set
    tax_category_id = frappe.get_doc("Code List", TAX_CATEGORY_CODE_LIST).get_code_for(
        party.doctype, party.name
    )

Code List should have a default code, so that it is easier to fetch:

    tax_id_type = frappe.get_doc("Common Code", party.tax_id_type or tax_type_codelist.default)

Would be handy too (.get_doc proxy):

    tax_id_type = codelist.get_doc(party.tax_id_type)  # raises
    tax_id_type = codelist.get_doc(party.tax_id_type, None)
    tax_id_type = codelist.get_doc(party.tax_id_type, codelist.default)

Ultimately, what I want is this, to keep my implementation at the leaf as clean as possible:

from .code_list import substitute


customer = frappe.get_doc("Customer", "Foo")

customer_for_render = substitute(customer)
# replaces all links to Common Code to the respective common code
# and infers defaults based on the field's filter
# {"code_list": "My List"} - take default of that list, if there is

If my edge case invalidates a default, I can still override it in sitiu

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.

2 participants