I think this should probably be a separate property like a supplierDisplayName
or handle it on the frontend

    {
      Header: <Translate id="react.productSupplier.column.supplier.label" defaultMessage="Supplier" />,
      accessor: 'supplierName',
      minWidth: 300,
      Cell: (row) => (
        <TableCell
          {...row}
          value={`${row.value} ${row.oginal.code ? `(${row.oginal.code})` : ''}`}
          tooltip
        />
      ),
    },

what do you think?

The code here is not the same as the supplier.code, hence I'd have to add another property to the toJson() anyway.

I followed what is done for this thing on the PO list:

origin: orderSummary?.order?.origin?.name + (origOrgCode ? " (${origOrgCode})" : ""),

I see... you are right, I did not notice that code references ProductSupplier code and not organization code.
Naming is a little confusing...

@drodzewicz yeah, understanding all the properties and their references also drove me nuts there, especially the supplier(ref to Organization) vs supplierName (String) vs supplierCode (String), manufacturer (ref to Organization), manufacturerName (String), manufacturerCode (String)

@kchelstowski For what it's worth, I wasn't ignoring your comment in the other PR. I just can't keep up with the volume of tickets / PRs that @ me on a daily basis. This was one of the 100+ emails (now 200) that was in my queue that I would have eventually gotten to, but didn't get to before leaving on vacation.

So apologies, this is why I can't be involved in the majority of code reviews. For now, please @ me in Slack with a link to the PR if I haven't responded in a timely manner (that goes for everyone).

Hopefully, we can get our conventions in a better place so that I'm no longer the bottleneck on these issues. But please keep me in the loop when there's a convention conversation that needs to be resolved and documented.

cc @alannadolny @kchelstowski @awalkowiak @drodzewicz

@jmiranda no worries. As I wrote above and explained on the tech huddle - the point of that comment was to say, that I've had the same feeling about this toJson() as you do, and this is why I also raised that it in the PR from the past.
This conversation would anyway be raised again by me in the "big PR".
I hope you haven't treated my previous comment as a regret :)

So should I refactor all the eventual nested properties that are inside this toJson()

Yes, for product supplier, clean it all up. For existing toJson() methods that we stumble upon we can add the new objects without removing the existing properties, but mark the obsolete properties as deprecated somehow (convention we need to figure out.

I mean - e.g. currently we have productCode, so should I change it also to:

product: [
  code: product.code,
]

First of all, to be very clear here

productSupplier.code != product.productCode

So, if we end up needing to include most of the properties from these child objects then the toJson should look like this

code: code,
product: product,
supplier: supplier,
...

Or use the toBaseJson() method (if it exists) to return

code: code,
product: product.toBaseJson()
supplier: supplier.toBaseJson(),
...

But as I said on the call, it's probably fine to sprinkle maps in the toJson() for ProductSupplier and ProductSummary where we need to override the default toJson() of the child objects. Basically, we need to use the temporary map solution for child objects where we were adding new properties (like displayName) that do not need to exist in the target domain object's toJson() and probably shouldn't be there, at least until we come up with a good convention.

id: id,
code: code,
product: [
    id: product.id
    productCode: product.productCode,
    name: product.name,
    ...
],
supplier: [
   id: supplier.id
   code: supplier.code,
   name: supplier.name,
   displayName: supplierName: supplier?.name + (supplier?.code ? " (${supplier.code})" : ""),
   ...
]

With that said, we'll probably create a convention where the displayName property (or something like that) will be a standard property for all JSON responses, so I'm fine with either using the custom map approach or adding it to the default to*Json().

Thanks @jmiranda .
One more thing - because we distinguish the supplierCode which is a String field and the supplier.code.
Just supplierCode != supplier.code - so I assume the supplierCode should stay like it is now?

FWIW, one of my concerns with the toBaseJson() method (which is currently only used with the Location object) is that we'll eventually run into a casae where we need to add a new property but it doesn't make sense to add it to toBaseJson() but we also don't want to pull in all of the details for the product that comes along with toJson(), so now we need a completely separate representation that lies somewhere between toBaseJson() and toJson().

This is where the "different representations for JSON responses" comes in.
https://pihemr.atlassian.net/browse/OBPIH-2649

As @kchelstowski mentioned, there's potentially a solution using DTOs, but that's an implementation detail.

From the client's perspective I either need to go looking for a different resource

• /api/products
• /api/productSummaries
• /api/productDetails

Or I need to tell the API what I'm expecting back as a response by using the Accept header (and this probably needs to use a custom vnd.* mime-type, but I didn't want to waste time figuring out what to put as the value here).

• /api/products (assumes we want the default toJson() version)
• /api/products [Accept: application/json+base] (delegates to toBaseJson() version)
• /api/products [Accept: application/json+somewhereInBetween] (delegates to toSomewhereInBetweenJson() version)

or request parameter

• /api/products (assumes we want the default toJson() version)
• /api/products?presentation=base (delegates to toBaseJson() version)
• /api/products?presentation=somewhereInBetween (delegates to toSomewhereInBetweenJson() version)

We could even doing something more fancy

• /api/products.json (assumes we want the default toJson() version)
• /api/products.base.json (delegates to toBaseJson() version)
• /api/products.somewhereInBetween.xml (delegates to toSomewhereInBetweenXml() version)

Again, we haven't made up our minds yet but I'm finding more discussions that basically say there's no standard convention in REST, so ... it's up to us.

• https://stackoverflow.com/questions/6076602/rest-different-representations-of-the-same-data
• https://stackoverflow.com/questions/381568/rest-content-type-should-it-be-based-on-extension-or-accept-header

But still, @drodzewicz's point is valid, this should be moved to either supplierDisplayName or should be built on the front end

We should avoid having a JSON property return something different from the object's property value. In my mind the JSON can certainly be different, but I should be able to take the JSON object returned by the server, edit it, and post it back, and the server should be able to handle it.

The PO list approach was wrong. We should have returned an abbreviated copy of the Location object. In its JSON the Location object should have a "displayName" property that can encapsulate this concatenated value. For now we can do that in the marshallers and/or toJson methods using a map and return just the values we think we need.

origin: [
    name: orderSummary: order?.origin?.name,
    displayName: orderSummary?.order?.origin?.name + (origOrgCode ? " (${origOrgCode})" : "")
    ...
]

Do the same here. We can discuss on the tech huddle if necessary.

NOTE: This is going to be the way forward.

I want to think about this a little more. We've used this pattern twice now so it probably deserves it's own transient property getter or a utility.

Let's move it to a separate ticket or spike if needed. For now, I am merging this to the feature branch.