feat(spp_mis_demo_v2): add demo API client and statistics data#70
feat(spp_mis_demo_v2): add demo API client and statistics data#70
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the demo environment by introducing essential components for GIS integration and data visualization. It provides a ready-to-use API client for QGIS plugin testing and a set of predefined statistics that can be displayed on GIS maps and dashboards. These additions streamline the development and testing of features reliant on geographical data and aggregated metrics, ensuring a robust foundation for future enhancements. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (45.66%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #70 +/- ##
==========================================
+ Coverage 55.79% 57.70% +1.90%
==========================================
Files 162 178 +16
Lines 9291 11110 +1819
==========================================
+ Hits 5184 6411 +1227
- Misses 4107 4699 +592
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a demo API client and a set of statistical indicators, complete with data definitions and tests for the OpenSPP MIS Demo V2 module. However, two critical security vulnerabilities were identified in the pre-configured API client: API secrets are stored in plaintext in the database, and the demo client is granted excessive permissions (gis:all), violating the principle of least privilege. Beyond security, the implementation also contains critical issues that will prevent the module from installing or tests from passing, including an invalid scope in the API client data causing an XML loading error, and an incorrect method signature in new statistics tests leading to a TypeError. A minor inconsistency in the noupdate flag usage for new demo data files also needs to be addressed.
| <record id="demo_api_client_qgis" model="spp.api.client"> | ||
| <field name="name">QGIS Demo Client</field> | ||
| <field | ||
| name="description" | ||
| >Demo API client for testing QGIS plugin integration. Grants read access to GIS layers, reports, and statistics queries.</field> | ||
| <field name="partner_id" ref="base.main_partner" /> | ||
| <field name="organization_type_id" ref="spp_consent.org_type_government" /> | ||
| <field name="active" eval="True" /> | ||
| </record> |
There was a problem hiding this comment.
Creating an API client using the spp.api.client model results in the plaintext client_secret being stored in the database. This is because the client_secret field in the underlying model (defined in spp_api_v2/models/api_client.py) is a stored Char field and is not cleared after creation. This exposes sensitive API credentials to anyone with database access, significantly increasing the risk of unauthorized API access if the database is compromised.
| <!-- GIS Read Scope - View layers, catalog, statistics --> | ||
| <record id="demo_api_client_qgis_scope_gis_read" model="spp.api.client.scope"> | ||
| <field name="client_id" ref="demo_api_client_qgis" /> | ||
| <field name="resource">gis</field> |
There was a problem hiding this comment.
The resource value gis is not a valid option for the spp.api.client.scope model. This will cause an XML validation error when trying to install or update the module. The resource field is a Selection field that does not include gis as a choice. This issue is also present on line 42. You need to extend this selection in the appropriate module (e.g., spp_gis_report) to add gis as a valid option.
| result = self.aggregation_service.compute_aggregation( | ||
| registrant_ids=registrant_ids, statistics=[stat_name] | ||
| ) |
There was a problem hiding this comment.
The call to compute_aggregation is incorrect. The method signature is compute_aggregation(self, scope, ...), but you are passing registrant_ids as a keyword argument and omitting the required scope argument. This will cause a TypeError.
You need to construct a scope dictionary to pass the registrant IDs. Since you are querying groups, you should also specify an appropriate CEL profile.
scope = {
"scope_type": "cel",
"cel_expression": f"r.id in {registrant_ids}",
"cel_profile": "registry_groups",
}
result = self.aggregation_service.compute_aggregation(
scope=scope, statistics=[stat_name]
)| <record id="demo_api_client_qgis_scope_gis_all" model="spp.api.client.scope"> | ||
| <field name="client_id" ref="demo_api_client_qgis" /> | ||
| <field name="resource">gis</field> | ||
| <field name="action">all</field> | ||
| <field | ||
| name="description" | ||
| >Full GIS access including creating and managing geofences.</field> | ||
| </record> |
There was a problem hiding this comment.
The demo API client is granted the gis:all scope, which provides full administrative access to GIS resources, including geofence management. Following the Principle of Least Privilege, API clients should only be granted the minimum permissions necessary for their function. For a QGIS plugin integration, read-only access (gis:read) is typically sufficient. Granting excessive permissions increases the potential impact if the demo credentials are leaked or compromised.
| The separation allows the same underlying variable to be published | ||
| to multiple contexts with different presentation settings. | ||
| --> | ||
| <odoo noupdate="0"> |
There was a problem hiding this comment.
This file uses noupdate="0" at the <odoo> tag level, which means these demo records will be updated every time the module is updated. This can lead to user customizations being overwritten. For demo data, it's standard practice to use noupdate="1" to prevent this. I recommend changing the structure to be consistent with other demo files like demo_api_client.xml and to follow best practices:
<odoo>
<data noupdate="1">
<!-- all record definitions -->
</data>
</odoo>
Add demo data files for API testing and statistics indicators: - demo_api_client.xml: pre-configured QGIS API client with GIS scopes - demo_statistics.xml: 9 CEL variables + statistics for GIS/dashboard - test_demo_statistics.py: tests for statistics loading and aggregation Add spp_statistic, spp_aggregation, spp_studio as dependencies.
The demo_api_client.xml creates scopes with resource='gis', which is a selection value registered by spp_api_v2_gis. Without this dependency, the XML load fails with "Wrong value for spp.api.client.scope.resource: 'gis'".
…resh Port the geographic data pipeline from the internal branch: - Add load_geographic_data/country_code wizard fields and presets - Add _load_geographic_data() to load area shapes via spp_demo loader - Add _assign_registrant_areas() to assign municipalities to registrants - Add _generate_coordinates() for GPS points within area polygons - Add _refresh_gis_reports() to populate report data immediately - Add geographic data summary to success notification - Add spp_registrant_gis dependency for GPS coordinate field Without this, GIS reports are empty and QGIS plugin queries return no data.
Wrap demo_statistics.xml records in <data noupdate="1"> to prevent overwriting on module update, consistent with other demo files. Replace test_statistics_accessible_via_aggregation_service with test_statistics_have_valid_cel_accessors, which tests the correct property: that each statistic's variable has a cel_accessor configured. The original test used an incorrect compute_aggregation signature (registrant_ids kwarg instead of the required scope positional arg).
jeremi
force-pushed
the
feat/mis-demo-api-client
branch
from
200ce24 to
310f648
Compare
The _generate_coordinates method tried to read area.geo_polygon.data and pass it through wkbloads(), but the spp_gis ORM field already returns a Shapely geometry object which has no .data attribute. This caused a silent AttributeError for every area, resulting in zero coordinates generated for any registrant. Use the Shapely geometry directly instead of trying to re-parse it.
1 participant
Summary
demo_api_client.xml: pre-configured QGIS API client with GIS read/all scopesdemo_statistics.xml: 9 CEL variables + statistics for GIS/dashboard (total households, members, children, elderly, disabled, gender, enrollment)test_demo_statistics.py: tests for statistics loading, variable refs, GIS publishing, and aggregationspp_statistic,spp_aggregation,spp_studioas dependenciesTest plan
./scripts/test_single_module.sh spp_mis_demo_v2