From f447039bbed8f6c282d634c38ab3fca9d901c26e Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Sun, 23 Nov 2025 23:16:37 +0100 Subject: [PATCH] Fix tag layers restoration and filtering logic --- app/controllers/api/v1/places_controller.rb | 2 +- app/javascript/controllers/maps_controller.js | 84 +++++++++++-------- app/javascript/maps/places.js | 38 ++++----- app/javascript/maps/places_control.js | 25 +++--- app/models/concerns/omniauthable.rb | 10 +-- config/initializers/01_constants.rb | 2 +- spec/requests/shared/stats_spec.rb | 2 - 7 files changed, 84 insertions(+), 79 deletions(-) diff --git a/app/controllers/api/v1/places_controller.rb b/app/controllers/api/v1/places_controller.rb index d353f128..ae0c247b 100644 --- a/app/controllers/api/v1/places_controller.rb +++ b/app/controllers/api/v1/places_controller.rb @@ -65,7 +65,7 @@ module Api private def set_place - @place = current_api_user.places.find(params[:id]) + @place = current_api_user.places.includes(:tags, :visits).find(params[:id]) end def place_params diff --git a/app/javascript/controllers/maps_controller.js b/app/javascript/controllers/maps_controller.js index 49a8652b..bf8b9653 100644 --- a/app/javascript/controllers/maps_controller.js +++ b/app/javascript/controllers/maps_controller.js @@ -636,6 +636,21 @@ export default class extends BaseController { // Add event listeners for overlay layer changes to keep routes/tracks selector in sync this.map.on('overlayadd', (event) => { + // Track place tag layer restoration + if (this.isRestoringLayers && event.layer && this.placesFilteredLayers) { + // Check if this is a place tag layer being restored + const isPlaceTagLayer = Object.values(this.placesFilteredLayers).includes(event.layer); + if (isPlaceTagLayer && this.restoredPlaceTagLayers !== undefined) { + const tagId = event.layer._placeTagId; + this.restoredPlaceTagLayers.add(tagId); + + // Check if all expected place tag layers have been restored + if (this.restoredPlaceTagLayers.size >= this.expectedPlaceTagLayerCount) { + this.isRestoringLayers = false; + } + } + } + // Save enabled layers whenever a layer is added (unless we're restoring from settings) if (!this.isRestoringLayers) { this.saveEnabledLayers(); @@ -1784,42 +1799,30 @@ export default class extends BaseController { } }); - // Handle place tag layers (format: "place_tag:ID" or "place_tag:untagged") - const placeTagLayers = enabledLayers.filter(key => key.startsWith('place_tag:')); - if (placeTagLayers.length > 0) { - // Set flag once before restoring all place tag layers - this.isRestoringLayers = true; + // Place tag layers will be restored by updateTreeControlCheckboxes + // which triggers the tree control's change events to properly add/remove layers - placeTagLayers.forEach(layerKey => { - const tagId = layerKey.replace('place_tag:', ''); - let layer; + // Track expected place tag layers to be restored + const expectedPlaceTagLayers = enabledLayers.filter(key => key.startsWith('place_tag:')); + this.restoredPlaceTagLayers = new Set(); + this.expectedPlaceTagLayerCount = expectedPlaceTagLayers.length; - if (tagId === 'untagged') { - // Find untagged layer - layer = Object.values(this.placesFilteredLayers || {}).find(l => l._placeTagId === 'untagged'); - } else { - // Find layer by tag ID - const tagIdNum = parseInt(tagId); - layer = Object.values(this.placesFilteredLayers || {}).find(l => l._placeTagId === tagIdNum); - } - - if (layer && !this.map.hasLayer(layer)) { - layer.addTo(this.map); - console.log(`Enabled place tag layer: ${tagId}`); - } - }); - - // Reset flag after all layers have been added and events processed - setTimeout(() => { - this.isRestoringLayers = false; - console.log('[initializeLayersFromSettings] Finished restoring place tag layers'); - }, 150); - } + // Set flag to prevent saving during layer restoration + this.isRestoringLayers = true; // Update the tree control checkboxes to reflect the layer states + // The tree control will handle adding/removing layers when checkboxes change // Wait a bit for the tree control to be fully initialized setTimeout(() => { this.updateTreeControlCheckboxes(enabledLayers); + + // Set a fallback timeout in case not all layers get added + setTimeout(() => { + if (this.isRestoringLayers) { + console.warn('[initializeLayersFromSettings] Timeout reached, forcing restoration complete'); + this.isRestoringLayers = false; + } + }, 2000); }, 200); } @@ -1849,19 +1852,30 @@ export default class extends BaseController { // Check if this is a standard layer let shouldBeEnabled = enabledLayers.includes(layerName); - // Check if this is a place tag layer by finding the layer object - if (!shouldBeEnabled && this.placesFilteredLayers) { - const placeLayer = this.placesFilteredLayers[layerName]; + // Also check if this is a place tag layer + let placeLayer = null; + if (this.placesFilteredLayers) { + placeLayer = this.placesFilteredLayers[layerName]; if (placeLayer && placeLayer._placeTagId !== undefined) { - shouldBeEnabled = enabledTagIds.has(placeLayer._placeTagId); + // This is a place tag layer - check if it should be enabled + const placeLayerEnabled = enabledTagIds.has(placeLayer._placeTagId); + if (placeLayerEnabled) { + shouldBeEnabled = true; + } } } // Skip group headers that might have checkboxes if (layerName && !layerName.includes('Map Styles') && !layerName.includes('Layers')) { if (shouldBeEnabled !== input.checked) { - input.checked = shouldBeEnabled; - console.log(`Updated checkbox for ${layerName}: ${shouldBeEnabled}`); + // Checkbox state needs to change - simulate a click to trigger tree control + // The tree control listens for click events, not change events + input.click(); + } else if (shouldBeEnabled && placeLayer && !this.map.hasLayer(placeLayer)) { + // Checkbox is already checked but layer isn't on map (edge case) + // This can happen if the checkbox was checked in HTML but layer wasn't added + // Manually add the layer since clicking won't help (checkbox is already checked) + placeLayer.addTo(this.map); } } } diff --git a/app/javascript/maps/places.js b/app/javascript/maps/places.js index 2496bc8f..2fbecafa 100644 --- a/app/javascript/maps/places.js +++ b/app/javascript/maps/places.js @@ -25,7 +25,6 @@ export class PlacesManager { this.loadPlaces(); }); - console.log("[PlacesManager] Initializing, loading places for first time..."); await this.loadPlaces(); this.setupMapClickHandler(); this.setupEventListeners(); @@ -55,14 +54,19 @@ export class PlacesManager { }); } - async loadPlaces(tagIds = null) { + async loadPlaces(tagIds = null, untaggedOnly = false) { try { const url = new URL('/api/v1/places', window.location.origin); - if (tagIds && tagIds.length > 0) { + + if (untaggedOnly) { + // Load only untagged places + url.searchParams.append('untagged', 'true'); + } else if (tagIds && tagIds.length > 0) { + // Load places with specific tags tagIds.forEach(id => url.searchParams.append('tag_ids[]', id)); } + // If neither untaggedOnly nor tagIds, load all places - console.log("[PlacesManager] loadPlaces called, fetching from:", url.toString()); const response = await fetch(url, { headers: { 'Authorization': `Bearer ${this.apiKey}` } }); @@ -261,9 +265,9 @@ export class PlacesManager { } } - filterByTags(tagIds) { - this.selectedTags = new Set(tagIds); - this.loadPlaces(tagIds.length > 0 ? tagIds : null); + filterByTags(tagIds, untaggedOnly = false) { + this.selectedTags = new Set(tagIds || []); + this.loadPlaces(tagIds && tagIds.length > 0 ? tagIds : null, untaggedOnly); } /** @@ -278,11 +282,9 @@ export class PlacesManager { // Add event listener to load places when layer is added to map filteredLayer.on('add', () => { - console.log(`[PlacesManager] Filtered layer added to map, tagIds:`, tagIds); this.loadPlacesIntoLayer(filteredLayer, tagIds); }); - console.log(`[PlacesManager] Created filtered layer for tagIds:`, tagIds); return filteredLayer; } @@ -291,21 +293,20 @@ export class PlacesManager { */ async loadPlacesIntoLayer(layer, tagIds) { try { - console.log(`[PlacesManager] loadPlacesIntoLayer called with tagIds:`, tagIds); - let url = `/api/v1/places?api_key=${this.apiKey}`; + const url = new URL('/api/v1/places', window.location.origin); if (Array.isArray(tagIds) && tagIds.length > 0) { // Specific tags requested - url += `&tag_ids=${tagIds.join(',')}`; + tagIds.forEach(id => url.searchParams.append('tag_ids[]', id)); } else if (Array.isArray(tagIds) && tagIds.length === 0) { // Empty array means untagged places only - url += '&untagged=true'; + url.searchParams.append('untagged', 'true'); } - console.log(`[PlacesManager] Fetching from URL:`, url); - const response = await fetch(url); + const response = await fetch(url, { + headers: { 'Authorization': `Bearer ${this.apiKey}` } + }); const data = await response.json(); - console.log(`[PlacesManager] Received ${data.length} places for tagIds:`, tagIds); // Clear existing markers in this layer layer.clearLayers(); @@ -315,8 +316,6 @@ export class PlacesManager { const marker = this.createPlaceMarker(place); layer.addLayer(marker); }); - - console.log(`[PlacesManager] Added ${data.length} markers to layer`); } catch (error) { console.error('Error loading places into layer:', error); } @@ -330,14 +329,12 @@ export class PlacesManager { ensurePlacesLayerVisible() { // Check if the main places layer is already on the map if (this.map.hasLayer(this.placesLayer)) { - console.log('Places layer already visible'); return; } // Try to find and enable the Places checkbox in the tree control const layerControl = document.querySelector('.leaflet-control-layers'); if (!layerControl) { - console.log('Layer control not found, adding places layer directly'); this.map.addLayer(this.placesLayer); return; } @@ -356,7 +353,6 @@ export class PlacesManager { input.checked = true; input.dispatchEvent(new Event('change', { bubbles: true })); - console.log('Enabled Places layer in tree control'); // Reset the flag after a short delay to allow the event to process setTimeout(() => { diff --git a/app/javascript/maps/places_control.js b/app/javascript/maps/places_control.js index 1c485987..43b53bcc 100644 --- a/app/javascript/maps/places_control.js +++ b/app/javascript/maps/places_control.js @@ -171,21 +171,18 @@ export function createPlacesControl(placesManager, tags, userTheme = 'dark') { applyCurrentFilters: function() { if (!this.placesEnabled) return; - // If no specific filters, show all places - if (this.activeFilters.size === 0 && !this.showUntagged) { - this.placesManager.filterByTags(null); - } else { - // Build filter criteria - const tagIds = Array.from(this.activeFilters); + // Build filter criteria + const tagIds = Array.from(this.activeFilters); - // For now, just filter by tags - // TODO: Add support for untagged filter in PlacesManager - if (tagIds.length > 0) { - this.placesManager.filterByTags(tagIds); - } else if (this.showUntagged) { - // Show only untagged places - this.placesManager.filterByTags([]); - } + if (this.showUntagged && tagIds.length === 0) { + // Show only untagged places + this.placesManager.filterByTags(null, true); + } else if (tagIds.length > 0) { + // Show places with specific tags + this.placesManager.filterByTags(tagIds, false); + } else { + // Show all places (no filters) + this.placesManager.filterByTags(null, false); } }, diff --git a/app/models/concerns/omniauthable.rb b/app/models/concerns/omniauthable.rb index c29a559f..a9a9a325 100644 --- a/app/models/concerns/omniauthable.rb +++ b/app/models/concerns/omniauthable.rb @@ -15,23 +15,23 @@ module Omniauthable return user if user # If not found, try to find by email - user = find_by(email: data['email']) + user = find_by(email: data['email']) if data['email'].present? if user # Update provider and uid for existing user (first-time linking) user.update!(provider: provider, uid: uid) + return user end - # Create new user if not found - user = create( + return nil unless data['email'].present? + + create( email: data['email'], password: Devise.friendly_token[0, 20], provider: provider, uid: uid ) - - user end end end diff --git a/config/initializers/01_constants.rb b/config/initializers/01_constants.rb index c4d06e0a..34094edd 100644 --- a/config/initializers/01_constants.rb +++ b/config/initializers/01_constants.rb @@ -42,7 +42,7 @@ METRICS_PASSWORD = ENV.fetch('METRICS_PASSWORD', 'prometheus') OMNIAUTH_PROVIDERS = if SELF_HOSTED # Self-hosted: only OpenID Connect - ENV['OIDC_CLIENT_ID'].present? ? %i[openid_connect] : [] + (ENV['OIDC_CLIENT_ID'].present? && ENV['OIDC_CLIENT_SECRET'].present?) ? %i[openid_connect] : [] else # Cloud: only GitHub and Google providers = [] diff --git a/spec/requests/shared/stats_spec.rb b/spec/requests/shared/stats_spec.rb index aa8e704a..9c342e21 100644 --- a/spec/requests/shared/stats_spec.rb +++ b/spec/requests/shared/stats_spec.rb @@ -3,8 +3,6 @@ require 'rails_helper' RSpec.describe 'Shared::Stats', type: :request do - - context 'public sharing' do let(:user) { create(:user) } let(:stat) { create(:stat, :with_sharing_enabled, user:, year: 2024, month: 6) }