feat: implement dashboard export helpers#257
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull Request Overview
This PR implements new dashboard export and personalization helpers to support CSV/PDF exports and widget customization without external dependencies like reportlab. The changes refactor permission validation to use explicit checks with UserManagementService.usuario_tiene_permiso() and replace helper function calls with direct AuditoriaPermiso.objects.create() for audit logging.
Key changes:
- Added three new service methods:
ver_dashboard(),personalizar_dashboard(), andexportar_dashboard()for dashboard viewing, widget personalization, and export functionality - Removed dependency on
reportlabby implementing manual PDF generation - Refactored permission checking in existing
exportar(),personalizar(), andcompartir()methods to use directUserManagementServicecalls instead of helper functions
Comments suppressed due to low confidence (1)
api/callcentersite/callcentersite/apps/dashboard/services.py:315
- Test file
tests/dashboard/test_casos_uso_dashboard.py(lines 315, 348) callsDashboardService.compartir_dashboard(), but this method doesn't exist in the updated code. Only the legacycompartir()method exists (line 232). This will cause test failures. Either add the missingcompartir_dashboard()method or update the test files to callcompartir()instead.
def compartir(
usuario_id: int,
compartir_con_usuario_id: Optional[int] = None,
compartir_con_grupo_codigo: Optional[str] = None,
) -> Dict[str, object]:
"""
Comparte dashboard con otro usuario o grupo.
Args:
usuario_id: ID del usuario que comparte
compartir_con_usuario_id: ID del usuario receptor (opcional)
compartir_con_grupo_codigo: Codigo del grupo receptor (opcional)
Returns:
Diccionario con resultado:
- compartido_con: str (email o nombre de grupo)
- tipo: str ('usuario' o 'grupo')
- timestamp: str
Raises:
PermissionDenied: Si el usuario no tiene permiso
ValidationError: Si no se especifica receptor o no existe
Referencia: docs/PLAN_MAESTRO_PRIORIDAD_02.md (Tarea 30)
"""
tiene_permiso = UserManagementService.usuario_tiene_permiso(
usuario_id=usuario_id,
capacidad_codigo='sistema.vistas.dashboards.compartir',
)
if not tiene_permiso:
raise PermissionDenied('No tiene permiso para compartir dashboards')
# Validar que se especifico al menos un receptor
if not compartir_con_usuario_id and not compartir_con_grupo_codigo:
raise ValidationError(
'Debe especificar compartir_con_usuario_id o compartir_con_grupo_codigo'
)
compartido_con = None
tipo = None
# Compartir con usuario
if compartir_con_usuario_id:
try:
usuario_receptor = User.objects.get(
id=compartir_con_usuario_id,
is_deleted=False,
)
compartido_con = usuario_receptor.email
tipo = 'usuario'
except User.DoesNotExist:
raise ValidationError(
f'Usuario receptor no encontrado: {compartir_con_usuario_id}'
)
# Compartir con grupo
if compartir_con_grupo_codigo:
from callcentersite.apps.users.models_permisos_granular import GrupoPermiso
try:
grupo = GrupoPermiso.objects.get(codigo=compartir_con_grupo_codigo)
compartido_con = grupo.nombre
tipo = 'grupo'
except GrupoPermiso.DoesNotExist:
raise ValidationError(
f'Grupo no encontrado: {compartir_con_grupo_codigo}'
)
# TODO: Implementar logica real de compartir
# (crear registro en tabla compartidos, enviar notificacion, etc.)
AuditoriaPermiso.objects.create(
usuario_id=usuario_id,
capacidad_codigo='sistema.vistas.dashboards.compartir',
recurso_tipo='dashboard',
accion='compartir',
resultado='permitido',
detalles=f'Dashboard compartido con {tipo}: {compartido_con}',
)
return {
'compartido_con': compartido_con,
'tipo': tipo,
'timestamp': timezone.now().isoformat(),
}
|
|
||
| widgets_invalidos = [widget for widget in widgets if widget not in WIDGET_REGISTRY] | ||
| if widgets_invalidos: | ||
| raise ValidationError(f"Widget invalido: {', '.join(widgets_invalidos)}") |
There was a problem hiding this comment.
The error message contains a typo: "invalido" should be "inválido" (with accent). This is inconsistent with other error messages in the codebase that use proper Spanish spelling.
| raise ValidationError(f"Widget invalido: {', '.join(widgets_invalidos)}") | |
| raise ValidationError(f"Widget inválido: {', '.join(widgets_invalidos)}") |
| writer = csv.DictWriter(output, fieldnames=["type", "title", "value", "change", "period"]) | ||
| writer.writeheader() | ||
| for widget in widgets: | ||
| writer.writerow(widget) |
There was a problem hiding this comment.
The CSV export may fail if any widget dictionary doesn't contain all the expected fields ("type", "title", "value", "change", "period"). The writer.writerow(widget) call will raise a ValueError if keys are missing. Add validation to ensure widgets have all required fields, or use extrasaction='ignore' and handle missing keys gracefully.
| writer = csv.DictWriter(output, fieldnames=["type", "title", "value", "change", "period"]) | |
| writer.writeheader() | |
| for widget in widgets: | |
| writer.writerow(widget) | |
| fieldnames = ["type", "title", "value", "change", "period"] | |
| writer = csv.DictWriter(output, fieldnames=fieldnames, extrasaction='ignore') | |
| writer.writeheader() | |
| for widget in widgets: | |
| # Fill missing keys with empty string | |
| row = {key: widget.get(key, "") for key in fieldnames} | |
| writer.writerow(row) |
| def exportar_dashboard(usuario_id: int, formato: str = 'csv') -> Union[str, bytes]: | ||
| """Exporta el dashboard del usuario en formato CSV o PDF. | ||
|
|
||
| Args: | ||
| usuario_id: ID del usuario. | ||
| formato: Formato solicitado (``csv`` o ``pdf``). | ||
|
|
||
| Returns: | ||
| Cadena CSV cuando ``formato`` es ``csv`` o bytes que representan un | ||
| PDF cuando ``formato`` es ``pdf``. | ||
|
|
||
| Raises: | ||
| ValidationError: Si el formato es inválido. | ||
| """ | ||
| if formato not in {"csv", "pdf"}: | ||
| raise ValidationError("Formato invalido. Use csv o pdf") | ||
|
|
||
| dashboard = DashboardService.ver_dashboard(usuario_id=usuario_id) | ||
| widgets = dashboard.get("widgets", []) | ||
|
|
||
| if formato == "csv": | ||
| output = StringIO() | ||
| writer = csv.DictWriter(output, fieldnames=["type", "title", "value", "change", "period"]) | ||
| writer.writeheader() | ||
| for widget in widgets: | ||
| writer.writerow(widget) | ||
| return output.getvalue() | ||
|
|
||
| buffer = BytesIO() | ||
| buffer.write(b"%PDF-1.4\n%\xe2\xe3\xcf\xd3\n") | ||
| buffer.write(b"1 0 obj<< /Type /Catalog /Pages 2 0 R >>endobj\n") | ||
| buffer.write(b"2 0 obj<< /Type /Pages /Kids[3 0 R] /Count 1 >>endobj\n") | ||
| buffer.write(b"3 0 obj<< /Type /Page /Parent 2 0 R /MediaBox[0 0 300 144] /Contents 4 0 R >>endobj\n") | ||
| buffer.write(b"4 0 obj<< /Length 44 >>stream\nBT /F1 12 Tf 72 700 Td (Dashboard Export) Tj ET\nendstream endobj\n") | ||
| buffer.write(b"xref\n0 5\n0000000000 65535 f \n0000000010 00000 n \n0000000060 00000 n \n0000000113 00000 n \n0000000200 00000 n \ntrail\n<< /Size 5 /Root 1 0 R >>\nstartxref\n280\n%%EOF") | ||
| return buffer.getvalue() |
There was a problem hiding this comment.
The new method exportar_dashboard bypasses permission checks entirely, while the existing exportar method (line 88-93) includes explicit permission validation. This creates a security vulnerability where users can export dashboards without proper authorization by calling the new method instead of the legacy one. Add the same permission check using UserManagementService.usuario_tiene_permiso with capability code 'sistema.vistas.dashboards.exportar'.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| buffer.write(b"%PDF-1.4\n%\xe2\xe3\xcf\xd3\n") | ||
| buffer.write(b"1 0 obj<< /Type /Catalog /Pages 2 0 R >>endobj\n") | ||
| buffer.write(b"2 0 obj<< /Type /Pages /Kids[3 0 R] /Count 1 >>endobj\n") | ||
| buffer.write(b"3 0 obj<< /Type /Page /Parent 2 0 R /MediaBox[0 0 300 144] /Contents 4 0 R >>endobj\n") | ||
| buffer.write(b"4 0 obj<< /Length 44 >>stream\nBT /F1 12 Tf 72 700 Td (Dashboard Export) Tj ET\nendstream endobj\n") | ||
| buffer.write(b"xref\n0 5\n0000000000 65535 f \n0000000010 00000 n \n0000000060 00000 n \n0000000113 00000 n \n0000000200 00000 n \ntrail\n<< /Size 5 /Root 1 0 R >>\nstartxref\n280\n%%EOF") |
There was a problem hiding this comment.
The manually constructed PDF in lines 173-178 is incomplete and malformed:
- Missing proper xref table entries (incorrect format and incomplete offsets)
- The xref section has "trail\n" instead of "trailer"
- Missing proper EOF newline
- Object references and offsets don't match the actual byte positions
This will produce a corrupted PDF that most readers will fail to open. Consider using a lightweight PDF library like fpdf or weasyprint, or at minimum, construct a valid minimal PDF with correct xref offsets and trailer structure.
| widgets = [WIDGET_REGISTRY[widget].__dict__ for widget in widget_keys if widget in WIDGET_REGISTRY] | ||
|
|
||
| if not widgets: | ||
| widgets = [widget.__dict__ for widget in WIDGET_REGISTRY.values()] |
There was a problem hiding this comment.
Using widget.__dict__ on dataclass instances may not work as expected. Dataclasses don't automatically expose all their fields via __dict__ in all contexts. Use dataclasses.asdict(widget) instead for consistent behavior.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| def personalizar_dashboard(usuario_id: int, widgets: List[str]) -> DashboardConfiguracion: | ||
| """Guarda la lista de widgets habilitados para el usuario. | ||
|
|
||
| Args: | ||
| usuario_id: ID del usuario dueño de la configuración. | ||
| widgets: Lista de identificadores de widgets. | ||
|
|
||
| Raises: | ||
| ValidationError: Si la lista está vacía o contiene widgets inexistentes. | ||
| """ | ||
| if not widgets: | ||
| raise ValidationError('Debe proporcionar al menos un widget para personalizar el dashboard') | ||
|
|
||
| widgets_invalidos = [widget for widget in widgets if widget not in WIDGET_REGISTRY] | ||
| if widgets_invalidos: | ||
| raise ValidationError(f"Widget invalido: {', '.join(widgets_invalidos)}") | ||
|
|
||
| configuracion, _ = DashboardConfiguracion.objects.update_or_create( | ||
| usuario_id=usuario_id, | ||
| defaults={"configuracion": {"widgets": widgets}}, | ||
| ) | ||
|
|
||
| return configuracion | ||
|
|
||
| @staticmethod | ||
| def exportar_dashboard(usuario_id: int, formato: str = 'csv') -> Union[str, bytes]: | ||
| """Exporta el dashboard del usuario en formato CSV o PDF. | ||
|
|
||
| Args: | ||
| usuario_id: ID del usuario. | ||
| formato: Formato solicitado (``csv`` o ``pdf``). | ||
|
|
||
| Returns: | ||
| Cadena CSV cuando ``formato`` es ``csv`` o bytes que representan un | ||
| PDF cuando ``formato`` es ``pdf``. | ||
|
|
||
| Raises: | ||
| ValidationError: Si el formato es inválido. | ||
| """ | ||
| if formato not in {"csv", "pdf"}: | ||
| raise ValidationError("Formato invalido. Use csv o pdf") | ||
|
|
||
| dashboard = DashboardService.ver_dashboard(usuario_id=usuario_id) | ||
| widgets = dashboard.get("widgets", []) | ||
|
|
||
| if formato == "csv": | ||
| output = StringIO() | ||
| writer = csv.DictWriter(output, fieldnames=["type", "title", "value", "change", "period"]) | ||
| writer.writeheader() | ||
| for widget in widgets: | ||
| writer.writerow(widget) | ||
| return output.getvalue() | ||
|
|
||
| buffer = BytesIO() | ||
| buffer.write(b"%PDF-1.4\n%\xe2\xe3\xcf\xd3\n") | ||
| buffer.write(b"1 0 obj<< /Type /Catalog /Pages 2 0 R >>endobj\n") | ||
| buffer.write(b"2 0 obj<< /Type /Pages /Kids[3 0 R] /Count 1 >>endobj\n") | ||
| buffer.write(b"3 0 obj<< /Type /Page /Parent 2 0 R /MediaBox[0 0 300 144] /Contents 4 0 R >>endobj\n") | ||
| buffer.write(b"4 0 obj<< /Length 44 >>stream\nBT /F1 12 Tf 72 700 Td (Dashboard Export) Tj ET\nendstream endobj\n") | ||
| buffer.write(b"xref\n0 5\n0000000000 65535 f \n0000000010 00000 n \n0000000060 00000 n \n0000000113 00000 n \n0000000200 00000 n \ntrail\n<< /Size 5 /Root 1 0 R >>\nstartxref\n280\n%%EOF") | ||
| return buffer.getvalue() |
There was a problem hiding this comment.
The new methods personalizar_dashboard and exportar_dashboard lack audit logging, while the existing methods personalizar (line 219-227) and exportar (line 103-110) properly create AuditoriaPermiso records. This creates an inconsistent audit trail where some dashboard operations are logged and others aren't, making it difficult to track user actions and comply with security requirements. Add equivalent audit logging using AuditoriaPermiso.objects.create() for both new methods.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| def ver_dashboard(usuario_id: int) -> Dict[str, object]: | ||
| """Retorna la configuración del dashboard del usuario. | ||
|
|
||
| Si el usuario no tiene configuración personalizada se usan los widgets | ||
| por defecto registrados en ``WIDGET_REGISTRY``. | ||
| """ | ||
| configuracion = DashboardConfiguracion.objects.filter( | ||
| usuario_id=usuario_id | ||
| ).first() | ||
|
|
||
| widget_keys = configuracion.configuracion.get("widgets", []) if configuracion else list(WIDGET_REGISTRY.keys()) | ||
| widgets = [WIDGET_REGISTRY[widget].__dict__ for widget in widget_keys if widget in WIDGET_REGISTRY] | ||
|
|
||
| if not widgets: | ||
| widgets = [widget.__dict__ for widget in WIDGET_REGISTRY.values()] | ||
|
|
||
| return { | ||
| "widgets": widgets, | ||
| "last_update": timezone.now().isoformat(), | ||
| } |
There was a problem hiding this comment.
The new method ver_dashboard lacks permission checks, while it appears to be the main entry point for viewing dashboards. Based on the view layer (views.py line 31-38) and the legacy overview() method pattern, this should validate the 'sistema.vistas.dashboards.ver' permission using UserManagementService.usuario_tiene_permiso() before returning dashboard data. Without this check, any user can view any other user's dashboard by passing their ID.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@copilot open a new pull request to apply changes based on the comments in this thread |
2-Coatl
deleted the
feature/analyze-and-implement-api-according-to-docs/backend-13-01-59
branch
Summary
Testing
Codex Task