feature: rich Tefenua data graphql

[yhru]

No description provided.

[maatinito]

Hello,
Merci pour les modifications. On est parfaitement dans l'esprit Graphql, ca fonctionne bien :-)
Voici quelques retours:

1. Parcelles cadastralles: La surface est toujours vide.
   
   • c'est quoi la différence entre prefixe et section ? Est-ce que ca n'est pas la meme chose ?
2. Batiments :
   
   • est-ce que nom et description c'est pas la meme chose ?
   • pourquoi surfaceCalculée est toujours vide
3. Marqueurs
   
   • Pourquoi la description est vide ?
4. Zones
   
   • Pourquoi avoir une surfaceCalculée et une surface ? Et pas simplement Surface ?
5. Selection_utilisateur
   
   • qu'est ce qui motive le fait d'avoir une selection_utilisateur dans le schema graphql alors qu'il y a cadastre, batiment, zone, marqueurs ?

[claude]

Code Review: Rich TeFenua Data in GraphQL API

Summary

This PR enriches the GraphQL API for TeFenua geographic data by adding specialized types (Marqueur, Zone, Batiment) to better represent different geographic entities from French Polynesia's cadastral data. The changes improve type safety and data structure clarity.

---

✅ Strengths

1. Well-structured type hierarchy

The inheritance pattern is clean and follows DRY principles:

• MarqueurType (base with commune/ile fields)
• ZoneType extends MarqueurType (adds surface)
• BatimentType extends ZoneType (adds nom)

2. Excellent test coverage

spec/graphql/te_fenua_champ_spec.rb provides comprehensive test scenarios:

• Type resolution for different geometries (Point → Marqueur, Polygon → Zone)
• All geographic feature types (Parcelle, Batiment, Zone, Marqueur)
• MultiPolygon handling
• Source attribution (cadastre, batiment, selection_utilisateur)
• Empty data edge cases

3. Proper GraphQL schema updates

• Schema files correctly regenerated (schema.graphql, schema.json)
• Stored queries updated to include new fragment fields
• Type unions properly registered in api/v2/schema.rb

4. Smart type resolution logic

The resolve_type method in app/graphql/types/geo_area_type.rb:21-41 intelligently determines types based on:

• Source type (cadastre, batiment, selection_utilisateur)
• Champ type (TeFenuaChamp vs others)
• Geometry type (polygon vs point)
• Properties presence (building detection via is_building?)

---

⚠️ Issues & Recommendations

1. CRITICAL: ParcelleCadastrale fields changed from required to optional

Location: app/graphql/schema.graphql:3923-3938

Fields like commune, numero, section, surface, etc. were changed from String! to String (nullable).

Issue: This is a breaking change for API consumers who expect these fields to always be present.

Recommendation:

• If these fields can genuinely be null for TeFenua data, document this in the PR description
• Consider adding validation to ensure parcelles from cadastre source always have these fields
• Add migration notes for API consumers
• OR: Keep fields non-null and ensure data consistency in sync_geo_areas_from_value

# In app/models/champs/te_fenua_champ.rb:145
# Consider validating required fields for cadastre source
if source == GeoArea.sources.fetch(:cadastre)
  next unless feature[:properties][:numero].present? && 
              feature[:properties][:section].present?
end

2. ISSUE: Duplicate Marqueur entries in GraphQL possibleTypes

Location: app/graphql/schema.json:11570-11583

The GeoArea interface lists Marqueur three times in possibleTypes.

Fix: This appears to be a schema generation issue. Run:

bin/rails graphql:schema:dump

3. ISSUE: Inconsistent SelectionUtilisateur type

Location: app/graphql/types/geo_areas/selection_utilisateur_type.rb:4-6

This type no longer has any fields (removed commune, ile, etc.) and only implements the interface. This seems inconsistent with the new type model.

Questions:

• Is SelectionUtilisateur still used? (For non-TeFenua carte champs?)
• Should it have the same fields as MarqueurType?

Recommendation: Document when SelectionUtilisateur vs Marqueur/Zone types are used.

4. CODE QUALITY: Complex conditional in resolve_type

Location: app/graphql/types/geo_area_type.rb:21-41

The nested conditionals are hard to follow:

when GeoArea.sources.fetch(:selection_utilisateur)
  if object.champ.class.name == 'Champs::TeFenuaChamp'
    if object.polygon? || object.multipolygon?
      Types::GeoAreas::ZoneType
    else
      Types::GeoAreas::MarqueurType
    end
  else
    Types::GeoAreas::SelectionUtilisateurType
  end

Recommendation: Extract to a method for better readability:

def resolve_type(object, context)
  case object.source
  when GeoArea.sources.fetch(:cadastre)
    resolve_cadastre_type(object)
  when GeoArea.sources.fetch(:batiment)
    Types::GeoAreas::BatimentType
  when GeoArea.sources.fetch(:selection_utilisateur)
    resolve_selection_type(object)
  end
end

private

def resolve_cadastre_type(object)
  object.is_building? ? Types::GeoAreas::BatimentType : Types::GeoAreas::ParcelleCadastraleType
end

def resolve_selection_type(object)
  return Types::GeoAreas::SelectionUtilisateurType unless object.champ.is_a?(Champs::TeFenuaChamp)
  
  object.polygon? || object.multipolygon? ? Types::GeoAreas::ZoneType : Types::GeoAreas::MarqueurType
end

5. PERFORMANCE: N+1 query potential

Location: app/graphql/types/geo_area_type.rb:32

object.champ.class.name requires loading the associated champ. For queries returning many geo_areas, this could cause N+1 queries.

Recommendation: Use object.champ_type if available, or preload champs in the GraphQL resolver.

6. SECURITY: String comparison for class names

Using object.champ.class.name == 'Champs::TeFenuaChamp' is brittle and could break with code refactoring.

Better approach:

object.champ.is_a?(Champs::TeFenuaChamp)

7. MISSING: Position field not tested

The position field was added to TeFenuaChampType but isn't covered in the test suite.

Recommendation: Add test case:

describe 'position field' do
  it 'returns the position from value JSON' do
    champ.update(value: {
      position: { type: 'Point', coordinates: [-149.5, -17.5] }
    }.to_json)
    
    # Query and verify position field
  end
end

---

🔒 Security Considerations

✅ No obvious security issues
✅ No SQL injection vectors
✅ GraphQL schema properly typed
⚠️ Ensure position data is validated (GeoJSON format)

---

🚀 Performance Considerations

1. Surface calculation: For TeFenua champs, surface is calculated geometrically every time (app/models/geo_area.rb:207-211). Consider caching if this becomes a bottleneck.

2. JSON parsing: sync_geo_areas_from_value parses potentially large JSON documents. Consider background processing for large datasets.

---

📝 Recommendations Summary

Must Fix:

1. ✅ Document breaking change for ParcelleCadastrale nullable fields
2. ✅ Regenerate schema to fix duplicate Marqueur entries
3. ✅ Change class name check to use is_a? instead of string comparison

Should Fix:

4. Extract resolve_type conditionals into methods
5. Add test coverage for position field
6. Document SelectionUtilisateur vs Marqueur/Zone usage

Nice to Have:

7. Consider preloading champs to avoid N+1 queries
8. Add validation for required parcelle fields

---

✨ Overall Assessment

This is solid work with excellent test coverage and a well-thought-out type hierarchy. The main concerns are:

• Breaking API changes that need documentation
• Code clarity improvements in type resolution
• Minor performance optimizations

Approval Status: ⚠️ Approve with minor changes - Address the breaking change documentation and class name comparison issue before merging.

---

Great job following PF-specific conventions (ile, commune_associee) and maintaining comprehensive tests! 🎉