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

We should be more forgiving on unexpected types, format changes in the knowledge panel API. #900

Open
teolemon opened this issue Apr 8, 2024 · 1 comment

Comments

@teolemon
Copy link
Member

teolemon commented Apr 8, 2024

Description

  • We should be more forgiving on unexpected types, format changes in the knowledge panel API.
  • Any unexpected input will crash the whole product page (it will talk about a network effort) (even attributes, basic details…)

Proposed solutions

  • Handle HTML links in HTML content so that we can demo the SignalConso DGCCRF integration on Wednesday
  • Make the HTML attribute optional for the ACTION element (for now we added an empty HTML attribute to avoid the bug)
  • Add a new button element that accepts an URL as a parameter (in coordination with @stephanegigandet )
  • Make as many things optional as possible. Kill knowledge panel if they crash, but keep the rest of the JSON alive in the view.
  • Try to be granular in crashes (eg not showing invalid ones, showing others)

The new Report a product panel showed that we fail in a spectacular way for small reasons. Knowledge panels shouldn’t crash an entire product.

@monsieurtanuki
Copy link
Contributor

@teolemon Part of it probably concerns Smoothie and not directly off-dart.
Do you have examples of products with "unexpected" KPs?

Regarding your suggestions...

Handle HTML links in HTML content so that we can demo the SignalConso DGCCRF integration on Wednesday

Probably in Smoothie, but as far as I can see in the code (smooth_html_widget.dart) we already open links (used in KP text card, action card and table card). I've just tested and it does work (e.g. on a "informations missing about the ingredient origins" KP, click on the "producer" link)

Make the HTML attribute optional for the ACTION element (for now we added an empty HTML attribute to avoid the bug)

Easy fix, to be coded on off-dart (make the field optional) and in Smoothie (don't display the html if null).

Add a new button element that accepts an URL as a parameter (in coordination with @stephanegigandet )

Could be done easily, in off-dart then Smoothie.

Make as many things optional as possible. Kill knowledge panel if they crash, but keep the rest of the JSON alive in the view.

The thing is that we need a proper JSON structure for toJson/fromJson operations.
The easiest fix about that would be to store KPs as a simple String, to be decoded with kindness, and not as a JSON.

Try to be granular in crashes (eg not showing invalid ones, showing others)

I had a look at the KP mandatory fields, but they are not that many of them. Would the world be a much better place just making the rare mandatory fields as optional?

  • knowledge_panel.dart:
    • KnowledgePanel: nothing is mandatory
    • TitleElement: only title is mandatory
  • knowledge_panel_element.dart:
    • KnowledgePanelTextElement: only html is mandatory
    • KnowledgePanelImageElement: only url is mandatory
    • KnowledgePanelPanelGroupElement: only panel_ids is mandatory
    • KnowledgePanelPanelIdElement: only panel_id is mandatory
    • KnowledgePanelTableCell: only text is mandatory
    • KnowledgePanelTableRowElement: only values is mandatory
    • KnowledgePanelTableColumn: only text is mandatory
    • KnowledgePanelWorldMapElement: only pointers is mandatory
    • KnowledgePanelGeoPointer: nothing is mandatory
    • KnowledgePanelLatLng: both lat and lng are mandatory
    • KnowledgePanelTableElement: id, columns and rows are mandatory
    • KnowledgePanelActionElement: both html and actions are mandatory
    • KnowledgePanelElement: only element_type is mandatory

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants