Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
12 changes: 9 additions & 3 deletions src/components/app.vue
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ except according to the terms contained in the LICENSE file.
<!-- If the user's session is restored during the initial navigation, that
will affect how the navbar is rendered. -->
<navbar v-if="!$route.meta.standalone" v-show="routerReady"/>
<outdated-version/>
<alert id="app-alert"/>
<feedback-button v-if="showsFeedbackButton"/>
<!-- Specifying .capture so that an alert is not hidden immediately if it
Expand Down Expand Up @@ -48,7 +49,12 @@ import { loadAsync } from '../util/load-async';

export default {
name: 'App',
components: { Alert, Navbar, FeedbackButton: defineAsyncComponent(loadAsync('FeedbackButton')) },
components: {
Alert,
Navbar,
FeedbackButton: defineAsyncComponent(loadAsync('FeedbackButton')),
OutdatedVersion: defineAsyncComponent(loadAsync('OutdatedVersion'))
},
inject: ['alert', 'config'],
setup() {
const { visiblyLoggedIn } = useSessions();
Expand Down Expand Up @@ -87,14 +93,14 @@ export default {
},
methods: {
checkVersion() {
const previousVersion = this.centralVersion.data;
const previousVersion = this.centralVersion.versionText;
return this.centralVersion.request({
url: '/version.txt',
clear: false,
alert: false
})
.then(() => {
if (previousVersion == null || this.centralVersion.data === previousVersion)
if (previousVersion == null || this.centralVersion.versionText === previousVersion)
return false;

// Alert the user about the version change, then keep alerting them.
Expand Down
14 changes: 13 additions & 1 deletion src/components/home/news.vue
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,29 @@ except according to the terms contained in the LICENSE file.
<span>{{ $t('title') }}</span>
</template>
<template #body>
<iframe src="https://getodk.github.io/central/news.html" :title="$t('title')"></iframe>
<iframe :src="iframeSrc" :title="$t('title')"></iframe>
Copy link
Member

Choose a reason for hiding this comment

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

Now that src is dynamic, how about adding a test of it?

</template>
</page-section>
</template>

<script setup>
import { computed, inject } from 'vue';
import { useRequestData } from '../../request-data';
import PageSection from '../page/section.vue';

defineOptions({
name: 'HomeNews'
});

const { centralVersion } = useRequestData();


const container = inject('container');
const { i18n: globalI18n } = container;
const languageSubtag = computed(() => new Intl.Locale(globalI18n.locale).language);
Copy link
Member

Choose a reason for hiding this comment

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

Similar to OutdatedVersionBanner, I'm thinking that we could pass the full locale.


const iframeSrc = computed(() => `https://getodk.github.io/central/news.html?version=${centralVersion.data?.currentVersion}&lang=${languageSubtag.value}`);
Copy link
Member

Choose a reason for hiding this comment

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

What happens if !centralVersion.dataExists? Does it become ?version=undefined?

Maybe we don't need to pass ?version here? I don't think we'd want to reload news.html once the response comes in for version.txt. (The response would cause the URL to change.) How about passing something static like ?outdatedVersionWarning=false?


</script>

<style lang="scss">
Expand Down
114 changes: 114 additions & 0 deletions src/components/outdated-version-banner.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
<!--
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 v-if="showBanner" class="outdated-version-banner">
<iframe :src="iframeSrc" :title="$t('title')"></iframe>
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to add a test of the value of the src attribute?


<a class="btn btn-primary"
href="https://docs.getodk.org/central-upgrade/"
target="_blank" rel="noopener"
Copy link
Member

Choose a reason for hiding this comment

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

I think target="_blank" implies rel="noopener" in modern browsers.

v-tooltip.aria-describedby="$t('instructionsToUpgradeTooltip')">
{{ $t('instructionsToUpgrade') }}
</a>

<button class="btn btn-danger"
type="button"
v-tooltip.aria-describedby="$t('dismissTooltip')"
@click="dismiss">
{{ $t('dismiss') }}
</button>
</div>
</template>

<script setup>
import { computed, inject } from 'vue';
import { useRequestData } from '../request-data';
import { useSessions } from '../util/session';

const { visiblyLoggedIn } = useSessions();
Copy link
Member

Choose a reason for hiding this comment

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

useSessions() adds an event listener and does some other things, so it should only be called in App. Instead, you can inject('visiblyLoggedIn'). You could provide a value for it in the tests of the component.


const { centralVersion, currentUser } = useRequestData();

defineOptions({
name: 'OutdatedVersionBanner'
Copy link
Member

@matthew-white matthew-white Dec 12, 2024

Choose a reason for hiding this comment

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

This component is referenced as OutdatedVersion in App and load-async.js. I'm fine with either OutdatedVersion or (as here) OutdatedVersionBanner, but I think it'd be nice to use a single name. If OutdatedVersion seems best, how about renaming this file to outdated-version.vue?

});

const container = inject('container');
const { i18n: globalI18n } = container;
const languageSubtag = computed(() => new Intl.Locale(globalI18n.locale).language);
Copy link
Member

Choose a reason for hiding this comment

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

Could we pass along the whole globalI18n.locale instead of just the subtag? I think we'll use the Frontend Transifex for these translations, so it should be the same set of locales.

I want to leave open the possibility that Central supports Simplified Chinese one day (zh-Hans). Currently, we support Traditional Chinese (zh-Hant). The two have the same language subtag, which is part of why I think it'd be nice to send the whole locale.


const iframeSrc = computed(() => `https://sadiqkhoja.com/central/outdated-version.html?version=${centralVersion.data?.currentVersion}&lang=${languageSubtag.value}`);

const showBanner = computed(() => {
// user is not logged in or doesn't have ability to set config (implying not an admin)
if (!currentUser.dataExists || !visiblyLoggedIn || !currentUser.can('config.set')) return false;
if (!centralVersion.dataExists) return false;

// User has seen the warning in the last 30 days, so don't show it again
// 864E5 is the number of milliseconds in a day
const dismissDate = currentUser.preferences?.site?.outdatedVersionWarningDismissDate;
if (dismissDate && centralVersion.currentDate.getTime() < (dismissDate.getTime() + (864E5 * 30))) return false;

// Difference between current year and Central version year is less than 2
const centralVersionYear = Number(centralVersion.currentVersion.match(/^(\d{4})/)[1]);
const currentYear = centralVersion.currentDate.getFullYear();
if (currentYear - centralVersionYear < 2) return false;

return true;
});

const dismiss = () => {
currentUser.preferences.site.outdatedVersionWarningDismissDate = centralVersion.currentDate;
};
</script>

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

.outdated-version-banner {
background-color: $color-danger-light;
border: 1px solid $color-danger;
border-radius: 8px;
padding: 0 20px 20px 20px;
text-align: center;
width: 700px;
margin: 20px auto;
box-shadow: 0 5px 10px rgba(0, 0, 0, 0.2);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
box-shadow: 0 5px 10px rgba(0, 0, 0, 0.2);
box-shadow: $box-shadow-popover;


iframe {
border-width: 0;
height: 250px;
width: 600px;
margin: 0 auto;
}

.btn {
margin: 0 5px;
min-width: 100px;
}
}

</style>

<i18n lang="json5">
{
"en": {
// This is a title for the outdated version banner shown for the screenreaders only
"title": "Outdate Version",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"title": "Outdate Version",
"title": "Outdated Version",

"instructionsToUpgrade": "Instructions to upgrade",
"instructionsToUpgradeTooltip": "Click here to see the instructions to upgrade the Central",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"instructionsToUpgradeTooltip": "Click here to see the instructions to upgrade the Central",
"instructionsToUpgradeTooltip": "Click here to see instructions to upgrade Central",

"dismiss": "Dismiss for 30 days",
"dismissTooltip": "Click here to dismiss this warning for 30 days."
}
}
</i18n>
9 changes: 8 additions & 1 deletion src/request-data/resources.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,14 @@ export default ({ i18n, http }, createResource) => {
: configDefaults),
loaded: computed(() => config.dataExists && config.loadError == null)
}));
createResource('centralVersion');
createResource('centralVersion', () => ({
transformResponse: ({ data, headers }) =>
shallowReactive({
versionText: data,
currentVersion: data.match(/\(v(\d{4}[^-]*)/)[1],
currentDate: new Date(headers.get('date'))
})
}));
createResource('analyticsConfig', noargs(setupOption));
createResource('roles', (roles) => ({
bySystem: computeIfExists(() => {
Expand Down
15 changes: 15 additions & 0 deletions src/request-data/user-preferences/normalizers.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,21 @@ export class SitePreferenceNormalizer extends PreferenceNormalizer {
static projectSortMode(val) {
return ['alphabetical', 'latest', 'newest'].includes(val) ? val : 'latest';
}

static outdatedVersionWarningDismissDate(val) {
// Frontend to Backend
if (val instanceof Date) {
return val.toISOString();
}

// Backend to Frontend
const isoDateRegex = /^\d{4}-\d{2}-\d{2}([T](\d{2}:\d{2}:\d{2}(\.\d+)?Z)?)?$/;
Copy link
Member

Choose a reason for hiding this comment

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

I mentioned DateTime.fromISO().isValid in the tests below, but I want to be explicit that I don't think we should use it here. The luxon package is kind of heavy, so I want to avoid including it in the main bundle.

I could see it being helpful one day to have a utility function like isISODate().

Copy link
Member

Choose a reason for hiding this comment

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

I feel like there's one more ? here than I'd expect. Isn't this saying that you could have T on its own without numbers after it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed all ? as Data.toISOString() returns a simplified string which is always 24 or 27 character. In our case it will be always 24.

if (typeof (val) === 'string' && isoDateRegex.test(val)) {
return new Date(val);
Copy link
Member

Choose a reason for hiding this comment

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

Could it be possible to always return a string so that the return type is consistent? I know right now it's different for Backend vs. Frontend, but I think it'd be a little clearer if it was always the same.

}

return null;
}
}

export class ProjectPreferenceNormalizer extends PreferenceNormalizer {
Expand Down
4 changes: 4 additions & 0 deletions src/util/load-async.js
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,10 @@ const loaders = new Map()
/* webpackChunkName: "component-not-found" */
'../components/not-found.vue'
)))
.set('OutdatedVersion', loader(() => import(
/* webpackChunkName: "component-not-found" */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/* webpackChunkName: "component-not-found" */
/* webpackChunkName: "component-outdated-version" */

'../components/outdated-version-banner.vue'
)))
.set('ProjectFormAccess', loader(() => import(
/* webpackChunkName: "component-project-form-access" */
'../components/project/form-access.vue'
Expand Down
18 changes: 18 additions & 0 deletions transifex/strings_en.json
Original file line number Diff line number Diff line change
Expand Up @@ -3620,6 +3620,24 @@
}
}
},
"OutdatedVersionBanner": {
"title": {
"string": "Outdate Version",
"developer_comment": "This is a title for the outdated version banner shown for the screenreaders only"
},
"instructionsToUpgrade": {
"string": "Instructions to upgrade"
},
"instructionsToUpgradeTooltip": {
"string": "Click here to see the instructions to upgrade the Central"
},
"dismiss": {
"string": "Dismiss for 30 days"
},
"dismissTooltip": {
"string": "Click here to dismiss this warning for 30 days."
}
},
"Pagination": {
"action": {
"first": {
Expand Down