Skip to content
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.

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

Add feedback button #962

Merged
merged 8 commits into from
Apr 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 12 additions & 5 deletions src/components/app.vue
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ except according to the terms contained in the LICENSE file.
will affect how the navbar is rendered. -->
<navbar v-show="routerReady"/>
<alert id="app-alert"/>
<feedback-button v-if="showsFeedbackButton"/>
<!-- Specifying .capture so that an alert is not hidden immediately if it
was shown after the click. -->
<!-- eslint-disable-next-line vuejs-accessibility/click-events-have-key-events -->
Expand All @@ -26,6 +27,8 @@ except according to the terms contained in the LICENSE file.
</template>

<script>
import { defineAsyncComponent } from 'vue';

import { START_LOCATION } from 'vue-router';

import Alert from './alert.vue';
Expand All @@ -35,23 +38,27 @@ import useCallWait from '../composables/call-wait';
import useDisabled from '../composables/disabled';
import { useRequestData } from '../request-data';
import { useSessions } from '../util/session';
import { loadAsync } from '../util/load-async';

export default {
name: 'App',
components: { Alert, Navbar },
inject: ['alert'],
components: { Alert, Navbar, FeedbackButton: defineAsyncComponent(loadAsync('FeedbackButton')) },
inject: ['alert', 'config'],
setup() {
useSessions();
const { visiblyLoggedIn } = useSessions();
useDisabled();

const { centralVersion } = useRequestData();
const { callWait } = useCallWait();
return { centralVersion, callWait };
return { visiblyLoggedIn, centralVersion, callWait };
},
computed: {
routerReady() {
return this.$route !== START_LOCATION;
}
},
showsFeedbackButton() {
return this.config.showsFeedbackButton && this.visiblyLoggedIn;
},
},
created() {
this.callWait('checkVersion', this.checkVersion, (tries) =>
Expand Down
81 changes: 81 additions & 0 deletions src/components/feedback-button.vue
matthew-white marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
<!--
Copyright 2024 ODK Central Developers
See the NOTICE file at the top-level directory of this distribution and at
https://github.com/getodk/central-frontend/blob/master/NOTICE.

This file is part of ODK Central. It is subject to the license terms in
the LICENSE file found in the top-level directory of this distribution and at
https://www.apache.org/licenses/LICENSE-2.0. No part of ODK Central,
including this file, may be copied, modified, propagated, or distributed
except according to the terms contained in the LICENSE file.
-->
<template>
<div id="feedback-button">
<a :href="surveyLink" target="_blank"><span>{{ $t('feedback') }}</span></a>
</div>
</template>

<script setup>
import { computed } from 'vue';
import { useRoute } from 'vue-router';
import { queryString } from '../util/request';

defineOptions({
name: 'FeedbackButton'
});

const route = useRoute();

const surveyLink = computed(() => {
const query = queryString({
st: '9hflqGrodnpWyS8g69dbORJ3RD8jSQChTBMaf7zpE5gaPElyDtKgAjI79y$zqctG',
'd[/data/host]': window.location.host,
'd[/data/page]': route.name
});

return `https://data.getodk.cloud/-/single/JLdODTs84WWsgibw4JOh0krbDenObgi${query}`;
});
</script>

<style lang="scss">
@import '../assets/scss/variables';

#feedback-button span {
display: block;
background-color: white;
position: fixed;
z-index: 99999;
right: 0;
bottom: 100px;
height: 4em;
border: 2px solid;
border-color: #e3e4e4;
border-top-left-radius: 5px;
border-top-right-radius: 5px;
text-align: center;
line-height: 2em;
cursor: pointer;
transform-origin: 50% 50%;
transform: translateX(50%) rotate(-90deg);
transition: right 0.25s;

color: $color-accent-primary;
font-size: 1em;
padding: 0em 1.25em 0 1.25em;
}

#feedback-button span:hover,
#feedback-button:focus-within span {
right: 10px;
}
</style>

<i18n lang="json5">
{
"en": {
// Text for a hovering feedback button shown anchored to right of screen. If your language doesn't have a single word or short
// phrase for "feedback", consider something like "comments" or "reactions".
"feedback": "Feedback"
}
}
</i18n>
16 changes: 4 additions & 12 deletions src/components/navbar.vue
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ except according to the terms contained in the LICENSE file.
<router-link to="/" class="navbar-brand">ODK Central</router-link>
</div>
<div class="collapse navbar-collapse">
<navbar-links v-if="loggedIn"/>
<navbar-links v-if="visiblyLoggedIn"/>
<div class="navbar-right">
<a v-show="showsAnalyticsNotice" id="navbar-analytics-notice"
href="#" @click.prevent="analyticsIntroduction.show()">
Expand All @@ -34,7 +34,7 @@ except according to the terms contained in the LICENSE file.
<ul class="nav navbar-nav">
<navbar-help-dropdown/>
<navbar-locale-dropdown/>
<navbar-actions :logged-in="loggedIn"/>
<navbar-actions/>
</ul>
</div>
</div>
Expand Down Expand Up @@ -67,7 +67,7 @@ export default {
NavbarLinks,
NavbarLocaleDropdown
},
inject: ['config'],
inject: ['config', 'visiblyLoggedIn'],
setup() {
// The component does not assume that this data will exist when the
// component is created.
Expand All @@ -81,16 +81,8 @@ export default {
};
},
computed: {
// Usually once the user is logged in (either after their session has been
// restored or after they have submitted the login form), we render a fuller
// navbar. However, if after submitting the login form, the user is
// redirected to outside Frontend, they will remain on /login until they are
// redirected. In that case, we do not render the fuller navbar.
loggedIn() {
return this.currentUser.dataExists && this.$route.path !== '/login';
},
showsAnalyticsNotice() {
return this.config.showsAnalytics && this.loggedIn &&
return this.config.showsAnalytics && this.visiblyLoggedIn &&
this.canRoute('/system/analytics') && this.analyticsConfig.dataExists &&
this.analyticsConfig.isEmpty() &&
Date.now() - Date.parse(this.currentUser.createdAt) >= /* 14 days */ 1209600000;
Expand Down
10 changes: 2 additions & 8 deletions src/components/navbar/actions.vue
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ including this file, may be copied, modified, propagated, or distributed
except according to the terms contained in the LICENSE file.
-->
<template>
<li v-if="!loggedIn" id="navbar-actions">
<li v-if="!visiblyLoggedIn" id="navbar-actions">
<a href="#" @click.prevent>
<span class="icon-user-circle-o"></span>{{ $t('notLoggedIn') }}
</a>
Expand Down Expand Up @@ -44,13 +44,7 @@ import { useRequestData } from '../../request-data';

export default {
name: 'NavbarActions',
inject: ['container', 'alert', 'unsavedChanges'],
props: {
loggedIn: {
type: Boolean,
default: false
}
},
inject: ['container', 'alert', 'unsavedChanges', 'visiblyLoggedIn'],
setup() {
// The component does not assume that this data will exist when the
// component is created.
Expand Down
3 changes: 2 additions & 1 deletion src/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,6 @@ export default {
title: null,
body: null
},
oidcEnabled: process.env.VUE_APP_OIDC_ENABLED === 'true'
oidcEnabled: process.env.VUE_APP_OIDC_ENABLED === 'true',
showsFeedbackButton: false
};
4 changes: 4 additions & 0 deletions src/util/load-async.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,10 @@ const loaders = new Map()
/* webpackChunkName: "component-entity-upload" */
'../components/entity/upload.vue'
)))
.set('FeedbackButton', loader(() => import(
/* webpackChunkName: "component-feedback-button" */
'../components/feedback-button.vue'
)))
.set('FieldKeyList', loader(() => import(
/* webpackChunkName: "component-field-key-list" */
'../components/field-key/list.vue'
Expand Down
27 changes: 21 additions & 6 deletions src/util/session.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ hand-in-hand.
*/

import { START_LOCATION } from 'vue-router';
import { inject, onBeforeUnmount } from 'vue';
import { computed, inject, onBeforeUnmount, provide } from 'vue';

import { afterNextNavigation, forceReplace } from './router';
import { apiPaths, isProblem, requestAlertMessage } from './request';
Expand Down Expand Up @@ -194,13 +194,28 @@ const logOutAfterStorageChange = (container) => (event) => {

export const useSessions = () => {
const container = inject('container');
const id = setInterval(logOutBeforeSessionExpires(container), 15000);
const handler = logOutAfterStorageChange(container);
window.addEventListener('storage', handler);
const intervalId = setInterval(logOutBeforeSessionExpires(container), 15000);
const storageHandler = logOutAfterStorageChange(container);
window.addEventListener('storage', storageHandler);
onBeforeUnmount(() => {
clearInterval(id);
window.removeEventListener('storage', handler);
clearInterval(intervalId);
window.removeEventListener('storage', storageHandler);
});

/* visiblyLoggedIn.value is `true` if the user not only has all the data from
Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, nice!

login, but is also visibly logged in. An example of when the user has data,
but isn't visibly logged in is if the user has submitted the login form and is
being redirected to outside Frontend (which isn't instant). In that case, they
will remain on /login until the redirect is complete, and the navbar will not
change to reflect their login. */
const { router, requestData } = container;
const { currentUser } = requestData;
const visiblyLoggedIn = computed(() => currentUser.dataExists &&
router.currentRoute.value !== START_LOCATION &&
router.currentRoute.value.path !== '/login');
Comment on lines +213 to +215
Copy link
Member

Choose a reason for hiding this comment

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

I adapted visiblyLoggedIn from the loggedIn computed property in Navbar. I also added a check that the current route isn't START_LOCATION so that visiblyLoggedIn.value is false throughout the initial navigation. The initial navigation is async because it attempts to restore the session. During the navigation, we don't show the navbar, and this additional check means that the feedback button also won't be shown.

provide('visiblyLoggedIn', visiblyLoggedIn);

return { visiblyLoggedIn };
};

export const restoreSession = (session) =>
Expand Down
32 changes: 32 additions & 0 deletions test/components/app.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { logOut } from '../../src/util/session';

import { load } from '../util/http';
import { mockLogin } from '../util/session';
import FeedbackButton from '../../src/components/feedback-button.vue';

describe('App', () => {
describe('change in Central version', () => {
Expand Down Expand Up @@ -221,4 +222,35 @@ describe('App', () => {
app.should.alert();
});
});

describe('feedback button', () => {
it('is shown if a user is logged in and config is true', async () => {
const container = {
config: { showsFeedbackButton: true }
};
mockLogin();

const app = await load('/', { container });
app.findComponent(FeedbackButton).exists().should.be.true();
});

it('is hidden if a user is logged in and config is false', async () => {
const container = {
config: { showsFeedbackButton: false }
};
mockLogin();

const app = await load('/', { container });
app.findComponent(FeedbackButton).exists().should.be.false();
});

it('is hidden if no user is logged in and config is true', async () => {
const container = {
config: { showsFeedbackButton: true }
};

const app = await load('/login', { container }).restoreSession(false);
Copy link
Member Author

Choose a reason for hiding this comment

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

I am annoyed! This is different from loading '/' with restoreSession(false)? I'm pretty sure I had tried that.

Copy link
Member

Choose a reason for hiding this comment

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

They should be the same! restoreSession(false) is the key part. I just changed it to /login because navigating to / when you're logged out will send you to /login. Might as well go there right away haha.

app.findComponent(FeedbackButton).exists().should.be.false();
});
});
});
30 changes: 9 additions & 21 deletions test/components/navbar/actions.spec.js
Original file line number Diff line number Diff line change
@@ -1,35 +1,23 @@
import sinon from 'sinon';

import Navbar from '../../../src/components/navbar.vue';
import NavbarActions from '../../../src/components/navbar/actions.vue';

import { load } from '../../util/http';
import { mockLogin } from '../../util/session';
import { mockRouter } from '../../util/router';
import { mount } from '../../util/lifecycle';

describe('NavbarActions', () => {
it('indicates if the user is not logged in', () => {
const navbar = mount(Navbar, {
container: { router: mockRouter('/login') },
global: {
// Stubbing AnalyticsIntroduction because of its custom <router-link>
stubs: { AnalyticsIntroduction: true }
}
});
const text = navbar.getComponent(NavbarActions).get('a').text();
text.should.equal('Not logged in');
});
it('indicates if the user is not logged in', () =>
load('/login')
Comment on lines -12 to +10
Copy link
Member

Choose a reason for hiding this comment

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

I also could have patched the test by injecting visiblyLoggedIn. However, I think it's a better test to call load() and mount App. That way, the test will actually use visiblyLoggedIn as provided by App.

.restoreSession(false)
.afterResponses(app => {
const text = app.getComponent(NavbarActions).get('a').text();
text.should.equal('Not logged in');
}));

it("shows the user's display name", async () => {
mockLogin({ displayName: 'Alice Allison' });
const navbar = mount(Navbar, {
container: { router: mockRouter('/') },
global: {
stubs: { AnalyticsIntroduction: true }
}
});
const a = navbar.getComponent(NavbarActions).get('a');
const app = await load('/');
const a = app.getComponent(NavbarActions).get('a');
a.text().should.equal('Alice Allison');
await a.get('span:nth-child(2)').should.have.textTooltip();
});
Expand Down
18 changes: 7 additions & 11 deletions test/components/navbar/links.spec.js
Original file line number Diff line number Diff line change
@@ -1,23 +1,19 @@
import { RouterLinkStub } from '@vue/test-utils';

import Navbar from '../../../src/components/navbar.vue';
import NavbarLinks from '../../../src/components/navbar/links.vue';

import { load } from '../../util/http';
import { mockLogin } from '../../util/session';
import { mockRouter } from '../../util/router';
import { mount } from '../../util/lifecycle';

describe('NavbarLinks', () => {
it('does not render the links before login', () => {
const navbar = mount(Navbar, {
container: { router: mockRouter('/login') },
global: {
// Stubbing AnalyticsIntroduction because of its custom <router-link>
stubs: { AnalyticsIntroduction: true }
}
});
navbar.findComponent(NavbarLinks).exists().should.be.false();
});
it('does not render the links before login', () =>
load('/login')
.restoreSession(false)
.afterResponses(app => {
app.findComponent(NavbarLinks).exists().should.be.false();
}));

it('renders the correct links for a sitewide administrator', () => {
mockLogin();
Expand Down
Loading