Skip to content

Commit d9c127b

Browse files
edwhclaude
andcommitted
Clear the remaining PR #744 TODOs: GroupsPage props, fetch dedup, stale comment
Three follow-ups to the GroupsRequiringModeration fix: 1. GroupsPage forwards showTags / networks / allGroupTags through to its inner GroupsTable on the 'Your groups' tab so tag badges actually render there (they didn't — the props were received from the blade but dropped on the floor). Also fixes a `your-area="yourArea"` literal-string bug (now `:your-area="yourArea"`). Removes three genuinely dead props (yourLat, yourLng, userId) and the matching blade attributes. Resolves the TODO at GroupsPage.vue:118. Added GroupsPage.test.js with 2 cases. 2. groups/fetch action now de-dups concurrent in-flight requests for the same (id, includeStats) key — GroupsPage's mounted() loop fires one fetch per yourGroup in parallel, so without this the same group could be in flight twice. Implemented as a module-scope Map; entry is cleared in a `finally` so a rejection doesn't poison the slot. Resolves the TODO at groups.js:227. Added groups.test.js with 5 cases (concurrent same-id, sequential, concurrent different-ids, error-then-retry, different includeStats). 3. Removed the stale 'See TODO below to chase the underlying reactivity bug separately' comment in grouptags.test.js — the reactivity bug WAS chased (unhandled async lifecycle rejections leaking Vue 2's scheduler `pending` flag) and the fix shipped in this PR (try/catch on async mounted hooks + flushRender() helper). Test infra: jest.config.json gets resources/js/store as a third root, plus moduleNameMapper stubs for leaflet-control-geocoder and vue-awesome so component tests can mount GroupsPage without resolving its transitive Leaflet/Icon deps. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent da1b055 commit d9c127b

9 files changed

Lines changed: 243 additions & 43 deletions

File tree

jest.config.json

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,8 @@
1212
},
1313
"roots": [
1414
"<rootDir>/resources/js/components",
15-
"<rootDir>/resources/js/misc"
15+
"<rootDir>/resources/js/misc",
16+
"<rootDir>/resources/js/store"
1617
],
1718
"modulePaths": [
1819
"<rootDir>"
@@ -23,6 +24,8 @@
2324
"setupFilesAfterEnv": ["<rootDir>/tests/jest.setup.js"],
2425
"testEnvironment": "jsdom",
2526
"moduleNameMapper": {
26-
"^resources/js/mixins/lang.js$": "<rootDir>/tests/__mocks__/resources/js/mixins/lang.js"
27+
"^resources/js/mixins/lang.js$": "<rootDir>/tests/__mocks__/resources/js/mixins/lang.js",
28+
"^leaflet-control-geocoder/.*$": "<rootDir>/tests/__mocks__/leaflet-control-geocoder.js",
29+
"^vue-awesome/.*$": "<rootDir>/tests/__mocks__/vue-awesome.js"
2730
}
2831
}
Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
import Vue from "vue"
2+
import { BootstrapVue } from 'bootstrap-vue'
3+
Vue.use(BootstrapVue)
4+
5+
import { mount, createLocalVue } from '@vue/test-utils'
6+
import Vuex from 'vuex'
7+
import LangMixin from 'resources/js/mixins/lang.js'
8+
import GroupsPage from './GroupsPage.vue'
9+
10+
const localVue = createLocalVue()
11+
localVue.use(Vuex)
12+
13+
function makeStore() {
14+
return new Vuex.Store({
15+
modules: {
16+
groups: {
17+
namespaced: true,
18+
getters: { list: () => [] },
19+
actions: { fetch: () => Promise.resolve() },
20+
},
21+
},
22+
})
23+
}
24+
25+
const groupsTableStub = {
26+
name: 'GroupsTable',
27+
props: {
28+
groupids: { type: Array },
29+
tab: { type: Number, default: 0 },
30+
yourArea: { type: String, default: null },
31+
networks: { type: Array, default: null },
32+
allGroupTags: { type: Array, default: null },
33+
showTags: { type: Boolean, default: false },
34+
},
35+
template: '<div class="stub-groups-table" />',
36+
}
37+
38+
const groupMapStub = { name: 'GroupMapAndList', template: '<div class="stub-map" />' }
39+
40+
function makeWrapper(props = {}) {
41+
return mount(GroupsPage, {
42+
localVue,
43+
store: makeStore(),
44+
mixins: [LangMixin],
45+
propsData: {
46+
yourGroups: [1, 2],
47+
nearbyGroups: [],
48+
networks: [{ id: 10, name: 'Test' }],
49+
allGroupTags: [{ id: 1, tag_name: 'Foo' }],
50+
...props,
51+
},
52+
stubs: { GroupsTable: groupsTableStub, GroupMapAndList: groupMapStub },
53+
})
54+
}
55+
56+
async function flushTabs(wrapper) {
57+
// b-tab has `lazy`, so the tab's content only renders after the tab activates
58+
// on the first Vue tick.
59+
await wrapper.vm.$nextTick()
60+
await wrapper.vm.$nextTick()
61+
}
62+
63+
test('forwards showTags / networks / allGroupTags to the inner GroupsTable so tag badges render on the "your groups" tab', async () => {
64+
const networks = [{ id: 10, name: 'Test' }]
65+
const allGroupTags = [{ id: 1, tag_name: 'Foo' }, { id: 2, tag_name: 'Bar' }]
66+
const wrapper = makeWrapper({ showTags: true, networks, allGroupTags })
67+
await flushTabs(wrapper)
68+
69+
const table = wrapper.findComponent(groupsTableStub)
70+
expect(table.exists()).toBe(true)
71+
expect(table.props('showTags')).toBe(true)
72+
expect(table.props('networks')).toEqual(networks)
73+
expect(table.props('allGroupTags')).toEqual(allGroupTags)
74+
})
75+
76+
test('forwards yourArea (bound, not the literal string "yourArea") to GroupsTable', async () => {
77+
const wrapper = makeWrapper({ yourArea: 'London' })
78+
await flushTabs(wrapper)
79+
80+
const table = wrapper.findComponent(groupsTableStub)
81+
expect(table.props('yourArea')).toBe('London')
82+
})

resources/js/components/GroupsPage.vue

Lines changed: 4 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,10 @@
3131
class="mt-3"
3232
:tab="currentTab"
3333
@nearest="currentTab = 1"
34-
your-area="yourArea"
34+
:your-area="yourArea"
35+
:networks="networks"
36+
:all-group-tags="allGroupTags"
37+
:show-tags="showTags"
3538
/>
3639
</div>
3740
<div v-else class="mt-2 mb-2 text-center" v-html="__('groups.no_groups_mine')" />
@@ -86,21 +89,6 @@ export default {
8689
required: false,
8790
default: null
8891
},
89-
yourLat: {
90-
type: String,
91-
required: false,
92-
default: null
93-
},
94-
yourLng: {
95-
type: String,
96-
required: false,
97-
default: null
98-
},
99-
userId: {
100-
type: Number,
101-
required: false,
102-
default: null
103-
},
10492
canCreate: {
10593
type: Boolean,
10694
required: false,
@@ -115,7 +103,6 @@ export default {
115103
type: Array,
116104
required: true
117105
},
118-
// TODO Check whether all these parameters are now used or can be removed
119106
allGroupTags: {
120107
type: Array,
121108
required: true

resources/js/store/groups.js

Lines changed: 32 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,10 @@ function getLocale() {
3131
return el.innerText.trim()
3232
}
3333

34+
// Module-scoped map of in-flight `groups/fetch` promises, keyed by
35+
// `${id}|${includeStats}`. Used by the action to de-dup concurrent callers.
36+
const inFlight = new Map()
37+
3438
export default {
3539
namespaced: true,
3640
state: {
@@ -224,21 +228,38 @@ export default {
224228
return id
225229
},
226230
async fetch({ rootGetters, commit }, params) {
227-
// TODO Handle fetching case.
228-
try {
229-
let url = '/api/v2/groups/' + params.id + '?api_token=' + rootGetters['auth/apiToken'] + '&locale=' + getLocale()
231+
// De-dup concurrent fetches for the same (id, includeStats) so callers
232+
// like GroupsPage's mounted loop don't fire N parallel requests when
233+
// the user has many groups. The cache key includes includeStats because
234+
// the two responses have different shapes.
235+
const key = params.id + '|' + (params.hasOwnProperty('includeStats') ? String(params.includeStats) : '')
236+
if (inFlight.has(key)) {
237+
return inFlight.get(key)
238+
}
230239

231-
if (params.hasOwnProperty('includeStats')) {
232-
url += '&includeStats=' + params.includeStats
233-
}
240+
const request = (async () => {
241+
try {
242+
let url = '/api/v2/groups/' + params.id + '?api_token=' + rootGetters['auth/apiToken'] + '&locale=' + getLocale()
243+
244+
if (params.hasOwnProperty('includeStats')) {
245+
url += '&includeStats=' + params.includeStats
246+
}
234247

235-
let ret = await axios.get(url)
248+
let ret = await axios.get(url)
236249

237-
commit('set', ret.data.data)
250+
commit('set', ret.data.data)
238251

239-
return ret.data.data
240-
} catch (e) {
241-
console.error("Group fetch failed", e)
252+
return ret.data.data
253+
} catch (e) {
254+
console.error("Group fetch failed", e)
255+
}
256+
})()
257+
258+
inFlight.set(key, request)
259+
try {
260+
return await request
261+
} finally {
262+
inFlight.delete(key)
242263
}
243264
}
244265
},

resources/js/store/groups.test.js

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
// Tests in-flight de-duplication of the groups/fetch action.
2+
//
3+
// The action is called eagerly by GroupsPage (one dispatch per yourGroups id)
4+
// and again whenever a user navigates to a group page. Without de-dup, the
5+
// same id can be in flight twice concurrently.
6+
7+
jest.mock('axios', () => ({
8+
__esModule: true,
9+
default: {
10+
get: jest.fn(),
11+
post: jest.fn(),
12+
patch: jest.fn(),
13+
delete: jest.fn(),
14+
},
15+
}))
16+
import axios from 'axios'
17+
18+
import groups from './groups'
19+
20+
// store/groups.js reads locale via document.getElementById('language-current').innerText.
21+
// jsdom doesn't populate innerText reliably, so set the property explicitly.
22+
beforeEach(() => {
23+
document.body.innerHTML = '<div id="language-current"></div>'
24+
const el = document.getElementById('language-current')
25+
Object.defineProperty(el, 'innerText', { value: 'en', configurable: true })
26+
axios.get.mockReset()
27+
})
28+
29+
function commit() {}
30+
const rootGetters = { 'auth/apiToken': 'TEST' }
31+
32+
function deferred() {
33+
let resolve, reject
34+
const promise = new Promise((res, rej) => { resolve = res; reject = rej })
35+
return { promise, resolve, reject }
36+
}
37+
38+
test('two concurrent fetches for the same group share a single in-flight request', async () => {
39+
const d = deferred()
40+
axios.get.mockReturnValueOnce(d.promise)
41+
42+
const a = groups.actions.fetch({ rootGetters, commit }, { id: 42 })
43+
const b = groups.actions.fetch({ rootGetters, commit }, { id: 42 })
44+
45+
expect(axios.get).toHaveBeenCalledTimes(1)
46+
47+
d.resolve({ data: { data: { id: 42, name: 'G' } } })
48+
await Promise.all([a, b])
49+
50+
expect(axios.get).toHaveBeenCalledTimes(1)
51+
})
52+
53+
test('a new fetch after the previous one settled hits the network again', async () => {
54+
axios.get.mockResolvedValueOnce({ data: { data: { id: 7, name: 'G7' } } })
55+
await groups.actions.fetch({ rootGetters, commit }, { id: 7 })
56+
expect(axios.get).toHaveBeenCalledTimes(1)
57+
58+
axios.get.mockResolvedValueOnce({ data: { data: { id: 7, name: 'G7 again' } } })
59+
await groups.actions.fetch({ rootGetters, commit }, { id: 7 })
60+
expect(axios.get).toHaveBeenCalledTimes(2)
61+
})
62+
63+
test('concurrent fetches for different groups each get their own request', async () => {
64+
const d1 = deferred()
65+
const d2 = deferred()
66+
axios.get.mockReturnValueOnce(d1.promise).mockReturnValueOnce(d2.promise)
67+
68+
const a = groups.actions.fetch({ rootGetters, commit }, { id: 1 })
69+
const b = groups.actions.fetch({ rootGetters, commit }, { id: 2 })
70+
71+
expect(axios.get).toHaveBeenCalledTimes(2)
72+
73+
d1.resolve({ data: { data: { id: 1 } } })
74+
d2.resolve({ data: { data: { id: 2 } } })
75+
await Promise.all([a, b])
76+
})
77+
78+
test('a fetch that throws clears its in-flight slot so retries can run', async () => {
79+
axios.get.mockRejectedValueOnce(new Error('boom'))
80+
await groups.actions.fetch({ rootGetters, commit }, { id: 9 })
81+
82+
// The previous fetch is no longer in flight, so a new one re-hits the network.
83+
axios.get.mockResolvedValueOnce({ data: { data: { id: 9 } } })
84+
await groups.actions.fetch({ rootGetters, commit }, { id: 9 })
85+
86+
expect(axios.get).toHaveBeenCalledTimes(2)
87+
})
88+
89+
test('fetches with the same id but different includeStats are independent requests', async () => {
90+
axios.get.mockResolvedValue({ data: { data: { id: 3 } } })
91+
92+
await groups.actions.fetch({ rootGetters, commit }, { id: 3, includeStats: false })
93+
await groups.actions.fetch({ rootGetters, commit }, { id: 3, includeStats: true })
94+
95+
expect(axios.get).toHaveBeenCalledTimes(2)
96+
expect(axios.get.mock.calls[0][0]).toContain('includeStats=false')
97+
expect(axios.get.mock.calls[1][0]).toContain('includeStats=true')
98+
})

resources/views/group/index.blade.php

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -62,10 +62,7 @@
6262
:your-groups="{{ json_encode($your_groups, JSON_INVALID_UTF8_IGNORE) }}"
6363
:nearby-groups="{{ json_encode($nearby_groups, JSON_INVALID_UTF8_IGNORE) }}"
6464
your-area="{{ $your_area }}"
65-
your-lat="{{ $your_lat }}"
66-
your-lng="{{ $your_lng }}"
6765
:can-create="{{ $can_create ? 'true' : 'false' }}"
68-
:user-id="{{ $myid }}"
6966
tab="{{ $tab }}"
7067
:network="{{ $network ? $network : 'null' }}"
7168
:networks="{{ json_encode($networks, JSON_INVALID_UTF8_IGNORE) }}"

tests/Integration/grouptags.test.js

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -37,16 +37,14 @@ async function getGroupId(page, baseURL) {
3737
return group.id
3838
}
3939

40-
// Helper that creates a tag against the network's API and reloads the network
41-
// page so the new tag is in `initialTags` from the blade template.
42-
//
43-
// We could (and originally did) drive the live Vue form, but Vue 2's render of
44-
// NetworkPage doesn't reliably re-render the .tag-item list after the FIRST
45-
// mutation from an empty `tags` array — the data is updated (verified via
46-
// $parent walk) but the v-if/v-show DOM doesn't reflect it. Subsequent
47-
// mutations from a non-empty starting state render correctly. We sidestep
48-
// that quirk by hitting the API and reloading. (See TODO below to chase the
49-
// underlying reactivity bug separately.)
40+
// Helpers that drive the network tag CRUD via the API directly. Tests assert
41+
// the resulting state after a page reload so they don't depend on Vue's
42+
// reactive list update at all — that path is already exercised end-to-end by
43+
// the NetworkPage component itself (with the flushRender() helper after each
44+
// mutation) and unit-tested separately. Keeping these tests at the API+reload
45+
// level makes them robust against the Vue 2 scheduler quirks we hit
46+
// originally (unhandled async lifecycle rejections leaking the `pending`
47+
// flag, fixed in this PR).
5048
async function getApiTokenFromPage(page) {
5149
const token = await page.evaluate(() => {
5250
let host = document.querySelector('.create-tag .tag-name-input') ||
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
// Stub for the leaflet-control-geocoder CommonJS subpath imports used by
2+
// GroupMap.vue. The real package is a runtime-only dependency; we never
3+
// exercise its behaviour from Jest, so any plain class is fine.
4+
class Stub {
5+
constructor() {}
6+
on() { return this }
7+
addTo() { return this }
8+
setQuery() {}
9+
}
10+
module.exports = { Geocoder: Stub, Photon: Stub, default: Stub }

tests/__mocks__/vue-awesome.js

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
// Stub for vue-awesome's Icon component (ES module export trips up babel-jest's
2+
// CJS transformer when imported transitively from a Vue component under test).
3+
module.exports = { name: 'fa-icon', template: '<span class="stub-fa-icon" />' }
4+
module.exports.default = module.exports

0 commit comments

Comments
 (0)