Skip to content

Chart: allow dynamic stacked - #30

Merged
ecarreras merged 5 commits into
mainfrom
allow-dynamic-stacked
Sep 28, 2026
Merged

ecarreras merged 5 commits into
mainfrom
allow-dynamic-stacked

Conversation

@ecarreras

@ecarreras ecarreras commented Jul 22, 2025 •

Copy link
Copy Markdown
Member

This pull request enhances the ooui/graph module by introducing support for grouping and processing data based on multiple fields, including the stacked attribute. It also adds comprehensive tests to validate the new functionality. The most important changes are grouped below:

Enhancements to Data Grouping and Processing:

  • Added a new method get_values_grouped_by_fields in ooui/graph/processor.py to enable grouping values by multiple fields. This method generates keys as tuples of field values and supports creating combined labels.
  • Updated the process method in ooui/graph/chart.py to handle cases where the stacked attribute is specified. It now uses get_values_grouped_by_fields when grouping by both label and stacked fields.
  • Adjusted the fields method in ooui/graph/chart.py to include the stacked attribute as a field if it is defined.

Bug Fixes and Improvements:

  • Replaced the use of y_field.stacked directly with a dynamically determined stacked variable in the process method to ensure the correct value is used in all cases.

Test Coverage:

  • Added new test cases in spec/graph/graph_spec.py to verify that the stacked attribute is treated as a field and that bar graphs with stacked fields are processed correctly. These tests validate the grouping logic and ensure the output matches the expected format. [1] [2]

Follow-up fix

  • Handles empty dynamic stack labels without changing the underlying grouping key.
  • Adds regression coverage for a selection field with stacked=False.
  • Validation: 145 examples passing with Python 3.11.

@ecarreras
ecarreras requested review from Copilot and eberloso July 22, 2025 08:37
@ecarreras ecarreras self-assigned this Jul 22, 2025
@ecarreras ecarreras added the enhancement New feature or request label Jul 22, 2025

This comment was marked as outdated.

@ecarreras ecarreras added the minor Create a Minor version label Jul 22, 2025
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@ecarreras
ecarreras requested a review from Copilot July 22, 2025 09:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request adds support for dynamic stacking in chart graphs by allowing the grouping of data based on multiple fields including the stacked attribute. The main enhancement enables charts to process stacked data dynamically rather than relying on static field references.

  • Introduces a new method get_values_grouped_by_fields to group data by multiple fields simultaneously
  • Updates chart processing logic to handle dynamic stacked field grouping when both label and stacked fields are present
  • Adds the stacked attribute as a recognized field in the fields collection

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
ooui/graph/processor.py Adds new get_values_grouped_by_fields method for multi-field grouping
ooui/graph/chart.py Updates field collection and processing logic to support dynamic stacked grouping
spec/graph/graph_spec.py Adds comprehensive test coverage for stacked field functionality
Comments suppressed due to low confidence (2)

ooui/graph/processor.py:45

  • The parameter name fields_names should be field_names to be consistent with the existing get_values_grouped_by_field function parameter naming.
def get_values_grouped_by_fields(fields_names, fields, values):

ooui/graph/processor.py:49

  • The docstring parameter name fields_names should be field_names to match the corrected parameter name.
    :param list fields_names: A list of field names by which to group values.

Comment thread ooui/graph/chart.py
Comment thread ooui/graph/chart.py
@ecarreras

Copy link
Copy Markdown
Member Author

@giscebot resolt conflictes i repassa la pull-request

Resolve the graph spec conflict while retaining both the dynamic stacked coverage and the newer indicator tests.\n\nCo-authored-by: ecarreras <294235+ecarreras@users.noreply.github.com>
@giscebot

Copy link
Copy Markdown
Contributor

Conflictes resolts i branca actualitzada amb main al commit 176f88f.

He conservat tant els tests de dynamic stacking com els nous tests d’indicadors de main. També he repassat el flux assenyalat als comentaris: quan stacked és un camp dinàmic s’agrupa explícitament per [label, stacked]; en cas contrari es manté l’agrupació tradicional per label.

Validació:

  • suite local: 144 exemples correctes amb Python 3.11
  • CI de GitHub: build correcte
  • PR fusionable, sense conflictes

@ecarreras
ecarreras requested a review from giscebot September 25, 2026 07:43

@giscebot giscebot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi ha un cas bloquejant abans de fusionar.

get_values_grouped_by_fields() concatena directament les etiquetes amb ' - '.join(...). get_value_and_label_for_field() retorna None per a un selection sense valor/fora de catàleg i per a un many2one buit, de manera que qualsevol registre amb el nou camp stacked no informat provoca TypeError: sequence item ... expected str instance, NoneType found i avorta tot el gràfic. L'agrupació anterior per un sol camp admetia aquests valors sense excepció.

Cal definir i preservar el tractament dels valors no informats (per exemple, normalitzant només la part de presentació a una cadena buida o a l'etiqueta corresponent, sense perdre la clau de grup) i afegir un test amb stacked=False per a un camp selection o many2one. El test ha de fallar amb la implementació actual i comprovar tant que no hi ha excepció com el resultat del grup.

He revisat el diff complet, el flux label + stacked, la interacció amb get_value_and_label_for_field() i els tests afegits. El check build és verd (144 exemples), però la suite local no s'ha pogut repetir en aquest host perquè el worktree no té instal·lades les dependències (lxml/mamba).

@ecarreras

Copy link
Copy Markdown
Member Author

@giscebot millora el codi per tenir en compte els errors que has trobat

@giscebot

Copy link
Copy Markdown
Contributor

No ho puc aplicar en aquest torn perquè el job ha arribat explícitament en mode review-only: no tinc autorització per modificar la branca, fer commit ni push.

El bloqueig continua vigent al HEAD 176f88f: get_values_grouped_by_fields() passa etiquetes potencialment None directament a ' - '.join(...). El següent pas és normalitzar només la representació de l'etiqueta (preservant la clau real d'agrupació) i afegir un test de regressió amb stacked=False en un selection o many2one, comprovant que no hi ha excepció i que el grup resultant és correcte.

Cal rellançar aquesta petició amb permisos d'implementació perquè pugui fer el canvi, provar-lo i pujar-lo a aquesta PR.

@ecarreras

Copy link
Copy Markdown
Member Author

@giscebot fes tu els canvis de la pull-request segons el que has detectat al fil d'aquesta pull-request

@giscebot

Copy link
Copy Markdown
Contributor

He revisat de nou el HEAD 176f88f arran d’aquesta petició. El defecte bloquejant continua present: get_values_grouped_by_fields() concatena etiquetes que poden ser None, i encara no hi ha el test de regressió amb un stacked buit.

No puc aplicar els canvis des d’aquest job perquè ha estat lliurat explícitament amb work_intent=review_only; això prohibeix editar, fer commit o push a la branca encara que el comentari demani implementació. La revisió formal CHANGES_REQUESTED es manté vigent i el build actual continua verd sobre el mateix commit.

Perquè ho implementi jo, cal reenviar/obrir el treball amb mode d’implementació sobre aquesta PR. El canvi esperat és normalitzar només l’etiqueta de presentació abans del join, preservar els valors reals de la tupla usada com a clau d’agrupació, i cobrir explícitament stacked=False.

@ecarreras

Copy link
Copy Markdown
Member Author

@giscebot fes tu els canvis de la pull-request segons el que has detectat al fil d'aquesta pull-request

Co-authored-by: ecarreras <294235+ecarreras@users.noreply.github.com>
@giscebot

Copy link
Copy Markdown
Contributor

Canvi aplicat al commit 1bfa8fd i pujat a la branca de la PR.

He corregit el cas bloquejant detectat a la revisió: get_values_grouped_by_fields() conserva els valors reals a la clau d’agrupació i normalitza només les etiquetes buides abans del join. També he afegit una regressió explícita per a un camp selection amb stacked=False, comprovant la clau ("Fruit", False) i l’etiqueta resultant.

Validació local: 23 exemples de processor_spec.py i suite completa de 145 exemples, tots correctes amb Python 3.11. El build de GitHub s’ha iniciat i encara és pendent.

@ecarreras
ecarreras requested a review from giscebot September 28, 2026 08:30

@giscebot giscebot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Aprovat. He revisat el diff complet i, en particular, el flux de fields() → agrupació per [label, stacked] → sortida del chart, la compatibilitat amb el comportament estàtic anterior i el tractament de valors buits.

El bloqueig de la revisió anterior queda resolt al HEAD 1bfa8fd: la clau d’agrupació conserva el valor False i només es normalitza l’etiqueta de presentació abans del join; el test de regressió ho comprova explícitament. També hi ha cobertura d’integració del chart dinàmic.

Validació: el check build de GitHub és verd amb 145 exemples. No he pogut repetir la suite en aquest host perquè el worktree no té mamba ni lxml, però he contrastat el resultat del CI amb el codi i la cobertura afegida. No hi veig regressions ni canvis pendents abans de fusionar.

@ecarreras
ecarreras merged commit 9fe7560 into main Sep 28, 2026
1 check passed
@ecarreras
ecarreras deleted the allow-dynamic-stacked branch September 28, 2026 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request minor Create a Minor version

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants