Skip to content

Fix race condition when changing teams while on the application page #4951

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Dec 20, 2024
Merged

Fix race condition when changing teams while on the application page #4951

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
59d9e13
Fix race condition when changing teams while on the application page
cstns Dec 19, 2024
5bf6a22
remove team dispatch
cstns Dec 19, 2024
5da7751
simplify team dispatch calls
cstns Dec 19, 2024
923aab4
restore team dispatch when changing teams
cstns Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions frontend/src/components/TeamSelection.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,12 +51,14 @@ export default {
methods: {
selectTeam (team) {
if (team) {
this.$router.push({
name: 'Team',
params: {
team_slug: team.slug
}
})
this.$store.dispatch('account/setTeam', team.slug)
.then(() => this.$router.push({
name: 'Team',
params: {
team_slug: team.slug
}
}))
.catch(e => console.warn(e))
}
},
createTeam () {
Expand Down
3 changes: 1 addition & 2 deletions frontend/src/mixins/Application.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,7 @@ export default {
this.application = await ApplicationApi.getApplication(applicationId)
// Check to see if we have the right team loaded
if (this.team?.slug !== this.application.team.slug) {
// Load the team for this application
await this.$store.dispatch('account/setTeam', this.application.team.slug)
return
}
const instancesPromise = ApplicationApi.getApplicationInstances(applicationId) // To-do needs to be enriched with instance state
const applicationInstances = await instancesPromise
Expand Down
14 changes: 8 additions & 6 deletions frontend/src/pages/account/Teams/Invitations.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,14 @@ export default {
await this.$store.dispatch('account/refreshTeams')
Alerts.emit(`Invite to "${invite.team.name}" has been accepted.`, 'confirmation')
// navigate to team dashboad once invite accepted
this.$router.push({
name: 'Team',
params: {
team_slug: invite.team.slug
}
})
this.$store.dispatch('account/setTeam', invite.team.slug)
.then(() => this.$router.push({
name: 'Team',
params: {
team_slug: invite.team.slug
}
}))
.catch(e => console.warn(e))
},
async rejectInvite (invite) {
await userApi.rejectTeamInvitation(invite.id, invite.team.id)
Expand Down
14 changes: 8 additions & 6 deletions frontend/src/pages/admin/Teams.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,12 +221,14 @@ export default {
this.loading = false
},
viewTeam (row) {
this.$router.push({
name: 'Team',
params: {
team_slug: row.slug
}
})
this.$store.dispatch('account/setTeam', row.slug)
.then(() => this.$router.push({
name: 'Team',
params: {
team_slug: row.slug
}
}))
.catch(e => console.warn(e))
}
}
}
Expand Down
24 changes: 5 additions & 19 deletions frontend/src/pages/application/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,6 @@ export default {
InstanceStatusPolling
},
mixins: [permissionsMixin, applicationMixin, instanceActionsMixin],
data: function () {
return {
mounted: false
}
},
computed: {
...mapState('account', ['features']),
navigation () {
Expand Down Expand Up @@ -106,20 +101,11 @@ export default {
return routes
}
},
async created () {
await this.updateApplication()

this.$watch(
() => this.$route.params.id,
async () => {
await this.updateApplication()
}
)
},
mounted () {
this.mounted = true
},
methods: {
watch: {
'team.name': {
handler: 'updateApplication',
immediate: true
}
}
}
</script>
18 changes: 10 additions & 8 deletions frontend/src/pages/team/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
</template>

<script>
import { useRoute } from 'vue-router'
import { mapGetters, mapState } from 'vuex'

import Loading from '../../components/Loading.vue'
Expand All @@ -40,7 +39,6 @@ export default {
TeamTrialBanner
},
async beforeRouteUpdate (to, from, next) {
await this.$store.dispatch('account/setTeam', to.params.team_slug)
// even if billing is not yet enabled, users should be able to see these screens,
// in order to delete the project, or setup billing
await this.checkRoute(to)
Expand Down Expand Up @@ -68,21 +66,25 @@ export default {
return this.isAdminUser || this.teamMembership?.role >= Roles.Viewer
}
},
watch: {
'$route.params.team_slug' (slug) {
this.$store.dispatch('account/setTeam', slug)
}
},
mounted () {
this.mounted = true
},
async beforeMount () {
await this.$store.dispatch('account/setTeam', useRoute().params.team_slug)
this.checkRoute(this.$route)
},
methods: {
checkRoute: async function (route) {
const allowedRoutes = [
'/team/' + this.team.slug + '/billing',
'/team/' + this.team.slug + '/settings',
'/team/' + this.team.slug + '/settings/general',
'/team/' + this.team.slug + '/settings/danger',
'/team/' + this.team.slug + '/settings/change-type'
'/team/' + this.team?.slug + '/billing',
'/team/' + this.team?.slug + '/settings',
'/team/' + this.team?.slug + '/settings/general',
'/team/' + this.team?.slug + '/settings/danger',
'/team/' + this.team?.slug + '/settings/change-type'
Comment on lines +83 to +87
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this not simply return [] if team is not set?

As it stands, you will return at array containing '/team/undefined/billing' etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Steve-Mcl have you tested it? I can confirm this PR fixes the bug that is live in production now that we should get fixed. I don't see any ill effects because of this.

Copy link
Contributor

@Steve-Mcl Steve-Mcl Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have you tested it?

Yes, it works well, however I only flagged this code section because the PR (on the whole) is about handling team being undefined. By the same virtue, if team IS undefined, then we will get a bunch of links to /undefined/

I am not blocking it - happy for the PR to be merged as is.

]
if (allowedRoutes.indexOf(route.path) === -1) {
// if we're on a path that requires billing
Expand Down
Loading