openfoodfoundation / sib-discovery-components

Development and application of Startin'Blox Discovery components, initial use in Discover Regenerative www.regenerative.org.au
1 stars 0 forks source link

Producer profile: supporting evidence summary (order and include Certifications from Assurance Type: Certification) #8

Closed mariocarabotta closed 1 month ago

mariocarabotta commented 2 months ago

This is looking great. Links at top of page link down to filtered evidence is working great Image

Issues:

It seems like the filters are only showing when there is evidence and the order is maybe changing? Image

In the mockup we had all the filters appearing regardless of results, and have a specific set order (as per below) The thing that is most important to us is consistency of the order, which should be as per the mockup below (note we're not concerned about the indentation)

Image

JbPasquier commented 2 months ago

It seems like the filters are only showing when there is evidence and the order is maybe changing?

I do compile the list of possible filters based on what I have on the resource.

They're not ordered, first to come first to be shown. I can sort them.

Doing [...] Done.

image

kirstenalarsen commented 2 months ago

Thank you! I also just noticed that the Source field is missing and the date format is different from designs / how we want i.e. dd/mm/yyyy

Design:

Image

Current

Image

JbPasquier commented 2 months ago

@mkllnk Can you point me which field is the source field?

About the date format, @mkllnk could it be handled on your side? If not, I can manage it on front-end but it may break if some date does not share the same format as others from the api. I've updated the date format to force the usage of the browser's locale one, so it should always be properly displayed to the end-user, as long as the api does serve a yyyy-mm-dd format.

Image

mkllnk commented 2 months ago

@JbPasquier I needed to add the source to the API. The field is ofn:Service_provider_name[0] in example: https://n8n.openfoodnetwork.org.uk/webhook/regen/assurance?id=recAmJx6YFmRH134j

Does that work?

JbPasquier commented 2 months ago

Does it needs to be an array? Should I expect receiving multiples sources?

Looking at the mock-up, it seems that it's a link to somewhere?

May I suggest you this format:

{
  "@id": "https://n8n.openfoodnetwork.org.uk/webhook/regen/assurance?id=recAmJx6YFmRH134j",
  "ofn:sources": [
    {
      "ofn:label": "ANU Sustainable Farms",
      "ofn:uri": "https://www.anu..../"
    },
    ...
  ],
...
}

Or, if only one source can be present, the same way as you did for wholesale urls:

{
  "@id": "https://n8n.openfoodnetwork.org.uk/webhook/regen/assurance?id=recAmJx6YFmRH134j",
  "ofn:source_label": "ANU Sustainable Farms",
  "ofn:source_uri": "https://www.anu..../",
...
}

A more specification-compliant approach would be:

{
  "@id": "https://n8n.openfoodnetwork.org.uk/webhook/regen/assurance?id=recAmJx6YFmRH134j",
  "ofn:source": {
    "@id": "https://..."
    "ofn:label": "ANU Sustainable Farms",
    "ofn:uri": "https://www.anu..../"
  },
...
}

But I understand that you'd want to avoid adding one more endpoint to your n8n's api.

mkllnk commented 2 months ago

Does it needs to be an array? Should I expect receiving multiples sources?

Only one source. The array comes from our data structure. Either you or I have to deal with it. :stuck_out_tongue:

I didn't know about the URL. I'll add it like your option 2.

mkllnk commented 2 months ago

Here you got your example: https://n8n.openfoodnetwork.org.uk/webhook/regen/assurance?id=recAmJx6YFmRH134j

"ofn:service_provider_label":   "ANU Sustainable Farms",
"ofn:service_provider_uri": "https://www.sustainablefarms.org.au/",
JbPasquier commented 2 months ago

Done!

While it's on n8n, it's still missing from the proxy https://api.regenerative.org.au/webhook/regen/assurance?id=recAmJx6YFmRH134j do you know how much time it usually takes to process the queue?

mkllnk commented 2 months ago

do you know how much time it usually takes to process the queue?

No. At the moment I have it set to refresh every 8 hours but even if I reduce that to 1 minute it doesn't update straight away. We don't have a good cache invalidation strategy at the moment. I'll set it to 1 hour at some point for production, I think.

kirstenalarsen commented 2 months ago

sorry, really irritating detail - can you please add the colon after Source @JbPasquier, so it is "Source: " , currently missing

Image

JbPasquier commented 2 months ago

Fixed

kirstenalarsen commented 2 months ago

@JbPasquier

JbPasquier commented 2 months ago

Current behaviour: I do compile the list from the ones that are shown then I check if there are any Certification, if so I add this filter. Then, I sort this list alphabetically.

If I do understand what you're asking me, you want this:

image

To become:

Am I right? I can do this.

I can't arbitrary put Certification in the middle of Supporting Evidence Classification's tags without edge cases. Like say:

Or I'll need some weird rules like "Put Certification right after the E". If it fits all of your use cases, I can do that too.

kirstenalarsen commented 2 months ago

It is fine for Certifications to be at the top The key thing here is that Certifications in the Summary should ONLY be reading from Assurance Type: Certification, there shouldn't be any data on the Supporting Evidence label Certification

So like on the Rosnay profile, see the Supporting Evidence which correctly includes their Biodynamic Certification

Image

BUT the summary currently doesn't

Also, please can the Summary be same order ie. Ecological ad the top, Other at the bottom

kirstenalarsen commented 2 months ago

Certification at the top is fine

kirstenalarsen commented 2 months ago

Rosnay currently not showing the Certification in the Summary

Image

JbPasquier commented 2 months ago

Also, please can the Summary be same order ie. Ecological ad the top, Other at the bottom

Please see the second part of this comment https://github.com/openfoodfoundation/sib-discovery-components/issues/8#issuecomment-2012136198

Should I force some custom sorting? It'll deeply bind the code with your datas. And so make this component only work for your datas from your API. Should I?


BUT the summary currently doesn't

Exact, the summary is only based on the supporting evidence source. The kind of mix-mashing we're doing on the bottom one really needs to be avoided, we're parking bikes and elephants on the same place.

From the API perspective there are no relation in any manner between a Certification and a Supporting Evidence object.

I'll see if I can reach something there.

kirstenalarsen commented 2 months ago

hmmm, it's the mismash I want :) Understand if it can't happen If it can happen then alphabetical is fine, just with Other at the bottom?

JbPasquier commented 2 months ago

Done.

Image

Image

mariocarabotta commented 2 months ago

this is looking great from my side.

leaving it to @kirstenalarsen for further testing

nice work!

kirstenalarsen commented 1 month ago

Yep looks good :)