[14.0][IMP] edi_oca: recompute exchange filename after getting related record #21

QuocDuong1306 · 2024-08-08T03:13:20Z

Context:

When a related record exists, {record_name} uses the display name of the related record to differentiate the filename appropriately.
When there is no related record (e.g., when creating an exchange record from storage or an endpoint), {record_name} uses the exchange record's display name.
- In this case, if {record_name} and {type.code} are both used in the filename pattern (e.g., "{record_name}-{type.code}-{dt}"), then the filename will end up with some redundant information when the exchange type name and code have similar slugs (e.g., "Test CSV Output" and "test_csv_output").
- To avoid that, we remove the type name from the exc record's display name when these conditions are met

Changes:

Remove the type name from the exc record's display name when a duplication of the type code occurs, utilizing the context to handle this adjustment.

chaule97 · 2024-08-12T07:54:58Z

edi_oca/tests/test_record.py

+            self.assertEqual(
+                record.exchange_filename,
+                "wood-corner-test_csv_output-2020-10-21-12-00-00.csv",
+            )


Suggested change

self.assertEqual(

record.exchange_filename,

"wood-corner-test_csv_output-2020-10-21-12-00-00.csv",

)

self.assertEqual(

record.exchange_filename,

f"{slugify(self.partner.display_name)}-test_csv_output-2020-10-21-12-00-00.csv",

)

thienvh332 · 2024-08-22T03:11:38Z

Hello @QuocDuong1306,
Can you be more specific about why we need this change in the description?

I'm not seeing the benefit of renaming here. Because even if it's a {record_name}-{type.code} use case, like your test case, it still contains the identifier in the file_name. It can still differentiate the file_name between 2 edi.exchange.record with the same exchange.record.type.

QuocDuong1306 · 2024-08-23T03:29:36Z

Can you be more specific about why we need this change in the description?

Did you have a look at the card (3395)?

Because even if it's a {record_name}-{type.code} use case, like your test case, it still contains the identifier in the file_name. It can still differentiate the file_name between 2 edi.exchange.record with the same exchange.record.type

Actually, we don't want the test-csv-output (exchange type) appear twice

thienvh332 · 2024-08-23T03:40:10Z

Thanks for your reply @QuocDuong1306 ,

I have no context for this issue if you don't mention it. So if possible please think about people like me.
About the problem of avoiding duplicate names with the same record_name and type.code, you can check and fix them right at the first calculation, right? Because if you recalculate when it is assigned to releated, it will be duplicated before it is assigned.

QuocDuong1306 · 2024-08-27T03:04:53Z

Hello a @thienvh332 , thanks for your suggestions, the PR has been updated, could you have a look?

thienvh332 · 2024-08-27T03:19:26Z

edi_oca/models/edi_exchange_record.py

@@ -130,7 +130,7 @@ def _compute_related_name(self):
            related_record = rec.record
            rec.related_name = related_record.display_name if related_record else ""

-    @api.depends("model", "type_id")
+    @api.depends("model", "res_id", "type_id")


If it is not possible to duplicate then I think there is no need to recalculate when res_id changes

Suggested change

@api.depends("model", "res_id", "type_id")

@api.depends("model", "type_id")

thienvh332

LGTM

nilshamerlinck

commit message seems outdated

nilshamerlinck · 2024-08-29T04:12:22Z

edi_oca/models/edi_exchange_record.py

-            name = "[{}] {}".format(rec.type_id.name, rec_name)
+            name = (
+                "[{}] {}".format(rec.type_id.name, rec_name)
+                if rec._context.get("display_exchange_type_name", True)


Suggested change

if rec._context.get("display_exchange_type_name", True)

if rec._context.get("include_exchange_type_name", True)

nilshamerlinck · 2024-08-29T04:12:30Z

edi_oca/models/edi_exchange_type.py

+        # Avoid duplicating type code in filename
+        if "{type.code}" in pattern and slugify(self.code) == slugify(self.name):
+            exchange_record = exchange_record.with_context(
+                display_exchange_type_name=False


Suggested change

display_exchange_type_name=False

include_exchange_type_name=False

nilshamerlinck · 2024-08-29T04:16:50Z

edi_oca/models/edi_exchange_type.py

@@ -222,6 +222,11 @@ def _make_exchange_filename(self, exchange_record):
        ext = self.exchange_file_ext
        pattern = pattern + ".{ext}"
        dt = self._make_exchange_filename_datetime()
+        # Avoid duplicating type code in filename
+        if "{type.code}" in pattern and slugify(self.code) == slugify(self.name):


this will be true in most cases:

wasn't the case occurring only without a related record?

yes nils, updated

Actually, this context will not affect if we go through a related record. But maybe needing a check for a related record is better

ok, can you improve the PR description to distinct the 2 cases:

when we have a related record, {record_name} will...

when we don't, ..., which can lead to redundant information in the filename due to...

also need to update commit name to reflect the new implementation

Thank nils, the PR is updated as suggested

nilshamerlinck · 2024-08-29T08:38:30Z

Without a related record (e.g., when creating an exchange record from storage or an endpoint), the exchange type name and code may appear similar when "slugified"

I think something is missing in that part about {record_name}

QuocDuong1306 · 2024-08-29T08:51:47Z

Hi nils, updated for that part

nilshamerlinck · 2024-08-29T09:20:25Z

In these cases, the exchange type name and code might become similar when "slugified" (e.g., "Test CSV Output" and "test_csv_output"), leading to redundant information in the filename if {record_name} and {type.code} are both used in the filename pattern (e.g., "{record_name}-{type.code}-{dt}").

I believe you are referring to the second case, right? So it should be: "In this case" (singular), and it should be indented under that case

In this case, if {record_name} and {type.code} are both used in the filename pattern (e.g., "{record_name}-{type.code}-{dt}"), then the filename will end up with some redundant information when the exchange type name and code have similar slugs (e.g., "Test CSV Output" and "test_csv_output").
- To avoid that, we remove the type name from the exc record's display name when these conditions are met

QuocDuong1306 · 2024-08-29T09:34:08Z

Very sensitive, thank nils, updated

trobz-bot bot added enhancement New feature or request Tests labels Aug 8, 2024

Repository owner deleted a comment from trobz-bot bot Aug 8, 2024

chaule97 reviewed Aug 12, 2024

View reviewed changes

QuocDuong1306 force-pushed the 14.0-imp-edi_oca-exchange-filename branch from 7d4e588 to 6808be3 Compare August 23, 2024 03:38

QuocDuong1306 force-pushed the 14.0-imp-edi_oca-exchange-filename branch from 6808be3 to 185be18 Compare August 27, 2024 02:59

thienvh332 reviewed Aug 29, 2024

View reviewed changes

QuocDuong1306 force-pushed the 14.0-imp-edi_oca-exchange-filename branch from 185be18 to 9963071 Compare August 29, 2024 03:21

thienvh332 approved these changes Aug 29, 2024

View reviewed changes

chaule97 approved these changes Aug 29, 2024

View reviewed changes

nilshamerlinck suggested changes Aug 29, 2024

View reviewed changes

QuocDuong1306 force-pushed the 14.0-imp-edi_oca-exchange-filename branch from 9963071 to 9f62e85 Compare August 29, 2024 04:41

[IMP] edi_oca: Prevent duplication of type code in exchange filenames

e9a26ed

QuocDuong1306 force-pushed the 14.0-imp-edi_oca-exchange-filename branch from 9f62e85 to e9a26ed Compare August 29, 2024 07:55

nilshamerlinck approved these changes Aug 29, 2024

View reviewed changes

QuocDuong1306 closed this Aug 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[14.0][IMP] edi_oca: recompute exchange filename after getting related record #21

[14.0][IMP] edi_oca: recompute exchange filename after getting related record #21

QuocDuong1306 commented Aug 8, 2024 •

edited

Loading

chaule97 Aug 12, 2024

thienvh332 commented Aug 22, 2024

QuocDuong1306 commented Aug 23, 2024

thienvh332 commented Aug 23, 2024

QuocDuong1306 commented Aug 27, 2024

thienvh332 Aug 27, 2024

thienvh332 left a comment

nilshamerlinck left a comment

nilshamerlinck Aug 29, 2024

nilshamerlinck Aug 29, 2024

nilshamerlinck Aug 29, 2024

QuocDuong1306 Aug 29, 2024 •

edited

Loading

nilshamerlinck Aug 29, 2024

QuocDuong1306 Aug 29, 2024 •

edited

Loading

nilshamerlinck commented Aug 29, 2024

QuocDuong1306 commented Aug 29, 2024

nilshamerlinck commented Aug 29, 2024

QuocDuong1306 commented Aug 29, 2024 •

edited

Loading

	@api.depends("model", "res_id", "type_id")
	@api.depends("model", "type_id")

	if rec._context.get("display_exchange_type_name", True)
	if rec._context.get("include_exchange_type_name", True)

	display_exchange_type_name=False
	include_exchange_type_name=False

[14.0][IMP] edi_oca: recompute exchange filename after getting related record #21

[14.0][IMP] edi_oca: recompute exchange filename after getting related record #21

Conversation

QuocDuong1306 commented Aug 8, 2024 • edited Loading

Context:

Changes:

chaule97 Aug 12, 2024

Choose a reason for hiding this comment

thienvh332 commented Aug 22, 2024

QuocDuong1306 commented Aug 23, 2024

thienvh332 commented Aug 23, 2024

QuocDuong1306 commented Aug 27, 2024

thienvh332 Aug 27, 2024

Choose a reason for hiding this comment

thienvh332 left a comment

Choose a reason for hiding this comment

nilshamerlinck left a comment

Choose a reason for hiding this comment

nilshamerlinck Aug 29, 2024

Choose a reason for hiding this comment

nilshamerlinck Aug 29, 2024

Choose a reason for hiding this comment

nilshamerlinck Aug 29, 2024

Choose a reason for hiding this comment

QuocDuong1306 Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

nilshamerlinck Aug 29, 2024

Choose a reason for hiding this comment

QuocDuong1306 Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

nilshamerlinck commented Aug 29, 2024

QuocDuong1306 commented Aug 29, 2024

nilshamerlinck commented Aug 29, 2024

QuocDuong1306 commented Aug 29, 2024 • edited Loading

QuocDuong1306 commented Aug 8, 2024 •

edited

Loading

QuocDuong1306 Aug 29, 2024 •

edited

Loading

QuocDuong1306 Aug 29, 2024 •

edited

Loading

QuocDuong1306 commented Aug 29, 2024 •

edited

Loading