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

CLDR-17560 Overall Errors #3743

Merged
merged 5 commits into from
May 24, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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
638 changes: 328 additions & 310 deletions tools/cldr-apps/js/package-lock.json

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion tools/cldr-apps/js/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
"ant-design-vue": "^3.2.16",
"browser-fs-access": "^0.35.0",
"marked": "^4.3.0",
"swagger-client": "^3.26.7",
"swagger-client": "^3.28.1",
srl295 marked this conversation as resolved.
Show resolved Hide resolved
"vue": "^3.2.47",
"xlsx": "https://cdn.sheetjs.com/xlsx-0.20.0/xlsx-0.20.0.tgz"
}
Expand Down
5 changes: 4 additions & 1 deletion tools/cldr-apps/js/src/esm/cldrClient.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import { SURVEY_TOOL_SESSION_HEADER } from "./cldrAjax.mjs";

const OAS3_ROOT = "/openapi"; // Path to the 'openapi' (sibling to cldr-apps). Needs to be a host-relative URL.

const RESOLVED_ROOT = new URL(OAS3_ROOT, document.baseURI);

/**
* Create a promise to a swagger client for ST operations.
*
Expand All @@ -16,7 +18,8 @@ const OAS3_ROOT = "/openapi"; // Path to the 'openapi' (sibling to cldr-apps). N
* @returns Promise<SwaggerClient>
*/
function getClient() {
return new SwaggerClient(OAS3_ROOT, {
return new SwaggerClient({
url: RESOLVED_ROOT,
requestInterceptor: (obj) => {
// add the session header to each request
obj.headers[SURVEY_TOOL_SESSION_HEADER] = getSessionId();
Expand Down
12 changes: 10 additions & 2 deletions tools/cldr-apps/js/src/esm/cldrComponents.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@ import { App } from "vue";
*/

// local components
import CldrError from "../views/CldrError.vue";
import CldrValue from "../views/CldrValue.vue";
import LoginButton from "../views/LoginButton.vue";
import OverallErrors from "../views/OverallErrors.vue";
import ReportResponse from "../views/ReportResponse.vue";
import SearchButton from "../views/SearchButton.vue";

Expand All @@ -19,6 +21,7 @@ import {
Alert,
Avatar,
Button,
Card,
Checkbox,
Collapse,
CollapsePanel,
Expand All @@ -31,9 +34,10 @@ import {
Select,
Spin,
Steps,
Tag,
Textarea,
Tooltip,
Timeline,
Tooltip,
UploadDragger,
} from "ant-design-vue";
// Note: 'notification' is a function and is imported as a function in cldrVue.mjs,
Expand All @@ -52,9 +56,10 @@ function setup(app) {
app.component("a-alert", Alert);
app.component("a-avatar", Avatar);
app.component("a-button", Button);
app.component("a-card", Card);
app.component("a-checkbox", Checkbox);
app.component("a-collapse", Collapse);
app.component("a-collapse-panel", CollapsePanel);
app.component("a-collapse", Collapse);
app.component("a-form-item", Form.Item);
app.component("a-form", Form);
app.component("a-input-password", Input.Password);
Expand All @@ -71,12 +76,15 @@ function setup(app) {
app.component("a-spin", Spin);
app.component("a-step", Steps.Step);
app.component("a-steps", Steps);
app.component("a-tag", Tag);
app.component("a-textarea", Textarea);
app.component("a-timeline-item", Timeline.Item);
app.component("a-timeline", Timeline);
app.component("a-tooltip", Tooltip);
app.component("a-upload-dragger", UploadDragger);
app.component("cldr-error", CldrError);
app.component("cldr-loginbutton", LoginButton);
app.component("cldr-overall-errors", OverallErrors);
app.component("cldr-report-response", ReportResponse);
app.component("cldr-searchbutton", SearchButton);
app.component("cldr-value", CldrValue);
Expand Down
11 changes: 11 additions & 0 deletions tools/cldr-apps/js/src/esm/cldrDash.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* cldrDash: encapsulate dashboard data.
*/
import * as cldrAjax from "./cldrAjax.mjs";
import * as cldrClient from "./cldrClient.mjs";
import * as cldrCoverage from "./cldrCoverage.mjs";
import * as cldrNotify from "./cldrNotify.mjs";
import * as cldrProgress from "./cldrProgress.mjs";
Expand Down Expand Up @@ -469,11 +470,21 @@ async function downloadXlsx(data, locale, cb) {
cb(null);
}

/**
* @param {string} locale locale to list for
* @returns {Array<CheckStatusSummary>}
*/
async function getLocaleErrors(locale) {
const client = await cldrClient.getClient();
return await client.apis.voting.getLocaleErrors({ locale });
}

export {
doFetch,
getFetchError,
saveEntryCheckmark,
setData,
updatePath,
downloadXlsx,
getLocaleErrors,
srl295 marked this conversation as resolved.
Show resolved Hide resolved
};
5 changes: 0 additions & 5 deletions tools/cldr-apps/js/src/esm/cldrSurvey.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -788,11 +788,6 @@ function testsToHtml(tests) {

newHtml += "</p>\n";
}
if (hadEntireLocale) {
Copy link
Member

Choose a reason for hiding this comment

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

this makes hadEntireLocale unused (dead code), so it should be removed above

Copy link
Member Author

Choose a reason for hiding this comment

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

true.

newHtml += `<p class='trInfo tr_Warning alert alert-warning fix-popover-help'>See also <a href='#r_supplemental/${cldrStatus.getCurrentLocale()}//'>${cldrText.get(
"special_r_supplemental"
)}</a></p>\n`;
}
return newHtml;
}

Expand Down
59 changes: 59 additions & 0 deletions tools/cldr-apps/js/src/views/CldrError.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
<template>
<div style="background: #ffffff; padding: 30px">
<a-card hoverable :title="subtypeString">
<template #extra
><a-tag :title="status.subtype" :color="status.type.toLowerCase()">{{
status.type
}}</a-tag></template
>
<p>
{{ status.message }}
</p>
<template v-if="status.subtypeUrl" #actions
><a class="warningReference" :href="status.subtypeUrl"
>How to Fix…</a
></template
>
</a-card>
</div>
</template>

<script>
// import * as cldrLoad from "../esm/cldrLoad.mjs";
// import * as cldrStatus from "../esm/cldrStatus.mjs";
Copy link
Member

Choose a reason for hiding this comment

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

commented-out code should be removed

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch,thanks

export default {
computed: {
myClass() {
return `cldrError tr_${this.status.type} `;
},
subtypeString() {
return this.status.subtype
.split(/(?=[A-Z])/)
.map((s) => s)
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand, what is the need for .map((s) => s)? Wouldn't the code do the same if that were removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

this was originally a lowercasing function. There's no titlecase in JS.

.join(" ");
},
},
props: {
status: {
type: Object,
Copy link
Member

Choose a reason for hiding this comment

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

is type an object, or a string? On the back end it's an enum, but in the json it looks very much like a string such as "Warning" -- or am I just totally confused?

Copy link
Member Author

@srl295 srl295 May 23, 2024

Choose a reason for hiding this comment

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

It's an object. In fact it's a serialized CheckStatusSummary.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we're talking about the same thing... status itself is an object, yes (I wrongly guessed CheckCLDR.CheckStatus but you say CheckStatusSummary and I believe you).

I was referring to status.type. On the back end that's CheckCLDR.CheckStatus.Type, which is an enum, and on the front end it's not an Object, it's a String. So, I think type: Object is wrong, it should be type: String

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, type: here is part of Vue, it's not status.type. See https://vuejs.org/guide/components/props.html compare to:

propE: { 
    type: Number:
    default: 100,
},

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 could have written:

props: {
    status: Object
}

// any additional classes
default: "",
Copy link
Member

Choose a reason for hiding this comment

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

should message, subtype and subtypeUrl be declared here? It looks like they're filled in from json from the back end. If that fails for any reason, the front end will throw cryptic errors like undefined subtype... I know we already have a lot of js code that makes this kind of implicit assumption. Still, objects with clearly defined and documented properties would be more ideal. I think status corresponds to back-end CheckCLDR.CheckStatus, and subtype corresponds to CheckCLDR.CheckStatus.Subtype. Comments could help...

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 don't think it will type check here.

},
},
};
</script>

<style scoped>
.subtype {
font-size: larger;
font-weight: bold;
color: black;
}

.cldrError {
border: 1px solid gray;
padding: 1em;
margin-bottom: 2em;
margin-right: 1em;
}
</style>
2 changes: 2 additions & 0 deletions tools/cldr-apps/js/src/views/GeneralInfo.vue
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
>
Open Dashboard
</button>

<cldr-overall-errors />
</div>
</template>

Expand Down
46 changes: 46 additions & 0 deletions tools/cldr-apps/js/src/views/OverallErrors.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
<template>
<a-spin v-if="!loaded" />
<div v-if="loaded && errors">
<h2>Overall Errors</h2>
<p>
Check the following errors carefully. These issues often require a ticket
to be filed with CLDR to fix.
</p>

<cldr-error v-for="test of errors" :key="test.subtype" :status="test" />
</div>
</template>

<script setup>
import { ref } from "vue";
import { getLocaleErrors } from "../esm/cldrDash.mjs";
import { getCurrentLocale } from "../esm/cldrStatus.mjs";
import * as cldrNotify from "../esm/cldrNotify.mjs";

const loaded = ref(false);
const errors = ref(null);
const locale = ref(getCurrentLocale());

async function loadData() {
const resp = await getLocaleErrors(locale.value);
if (!resp || !resp.body) {
errors.value = null;
} else {
const { body } = resp;
const { tests } = body;
errors.value = tests;
}
loaded.value = true;
}

loadData().then(
() => {},
(err) => {
console.error(err);
cldrNotify.exception(
err,
"Loading overall errors for " + getCurrentLocale()
);
}
);
</script>
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
package org.unicode.cldr.web.api;

import java.util.ArrayList;
import java.util.Collection;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import javax.ws.rs.*;
import javax.ws.rs.core.MediaType;
import javax.ws.rs.core.Response;
Expand Down Expand Up @@ -136,6 +140,34 @@ public Response getPage(
return VoteAPIHelper.handleGetOnePage(loc, session, page, xpstrid);
}

/** Array of status items. Only stores one example entry per subtype. */
public static final class EntireLocaleStatusResponse {
public EntireLocaleStatusResponse() {}

void addAll(Collection<CheckStatus> list) {
for (final CheckStatus c : list) {
add(c);
}
}

void add(CheckStatus c) {
if (!allSubtypes.contains(c.getSubtype())) {
tests.add(new CheckStatusSummary(c));
allSubtypes.add(c.getSubtype());
}
}

@Schema(description = "list of test results")
public List<CheckStatusSummary> tests = new ArrayList<>();

// we only want to store one example for each subtype.
private Set<CheckStatus.Subtype> allSubtypes = new HashSet<>();

boolean isEmpty() {
return tests.isEmpty();
}
}

public static final class RowResponse {

public static final class Row {
Expand Down Expand Up @@ -372,4 +404,40 @@ public CheckStatusSummary(CheckStatus checkStatus) {
this.entireLocale = checkStatus.getEntireLocale();
}
}

@GET
@Path("/{locale}/errors")
@Produces(MediaType.APPLICATION_JSON)
@Operation(summary = "Get overall errors", description = "Get overall errors for a locale")
@APIResponses(
value = {
@APIResponse(
responseCode = "200",
description = "Error results",
content =
@Content(
mediaType = "application/json",
schema =
@Schema(
implementation =
EntireLocaleStatusResponse.class))),
@APIResponse(responseCode = "204", description = "No errors in this locale"),
@APIResponse(
responseCode = "403",
description = "Forbidden, no access to this data"),
@APIResponse(responseCode = "404", description = "Locale does not exist"),
@APIResponse(
responseCode = "500",
description = "Internal Server Error",
content =
@Content(
mediaType = "application/json",
schema = @Schema(implementation = STError.class))),
})
public Response getLocaleErrors(
@Parameter(required = true, example = "br", schema = @Schema(type = SchemaType.STRING))
@PathParam("locale")
String loc) {
return VoteAPIHelper.handleGetLocaleErrors(loc);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import org.unicode.cldr.web.DataPage.DataRow.CandidateItem;
import org.unicode.cldr.web.SurveyException.ErrorCode;
import org.unicode.cldr.web.UserRegistry.User;
import org.unicode.cldr.web.api.VoteAPI.EntireLocaleStatusResponse;
import org.unicode.cldr.web.api.VoteAPI.RowResponse;
import org.unicode.cldr.web.api.VoteAPI.RowResponse.Row.Candidate;
import org.unicode.cldr.web.api.VoteAPI.VoteResponse;
Expand Down Expand Up @@ -82,6 +83,45 @@ static Response handleGetOneRow(
return handleGetRows(args);
}

static Response handleGetLocaleErrors(String loc) {
final SurveyMain sm = CookieSession.sm;
final CLDRLocale locale = CLDRLocale.getInstance(loc);
final STFactory factory = sm.getSTFactory();
if (!factory.getAvailableCLDRLocales().contains(locale)) {
// locale not found
return Response.status(404).build();
}

TestResultBundle test = factory.getTestResult(locale, DataPage.getSimpleOptions(locale));

CLDRFile cldrFile = factory.make(locale, true);

if (test != null) {
EntireLocaleStatusResponse resp = new VoteAPI.EntireLocaleStatusResponse();

// add any 'early' errors
resp.addAll(test.getPossibleProblems());

// add all non-path status
for (final String x : cldrFile) {
List<CheckStatus> result = new ArrayList<CheckStatus>();
test.check(x, result, cldrFile.getStringValue(x));
for (final CheckStatus s : result) {
if (s.getEntireLocale()) resp.add(s);
}
}

if (resp.isEmpty()) {
// nothing.
return Response.noContent().build();
}

return Response.ok(resp).build();
} else {
return Response.status(500, "could not load test data").build();
}
}

static Response handleGetOnePage(String loc, String session, String page, String xpstrid) {
ArgsForGet args = new ArgsForGet(loc, session);
if ("auto".equals(page) && xpstrid != null && !xpstrid.isEmpty()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -234,8 +234,6 @@ public static Chart forReport(final ReportId report, final String locale) {
switch (report) {
case personnames:
return new ChartPersonName(locale);
case supplemental:
return new ChartSupplemental(locale);
default:
return null;
}
Expand Down
Loading
Loading