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

[ReviewEntries] Allow senses w/o gloss but w/ definition #3536

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
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
1 change: 1 addition & 0 deletions public/locales/en/translation.json
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,7 @@
"deleteDisabled": "This was imported with data that The Combine doesn't handle, so it may not be deleted.",
"error": {
"gloss": "Glosses cannot be left blank",
"glossAndDefinition": "Glosses and definitions cannot both be left blank",
"senses": "Cannot save an entry with no senses",
"vernacular": "Vernacular cannot be left blank"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,9 @@ export default function EditDialog(props: EditDialogProps): ReactElement {
(state: StoreState) =>
state.currentProjectState.project.analysisWritingSystems
);
const definitionsEnabled = useAppSelector(
(state: StoreState) => state.currentProjectState.project.definitionsEnabled
);
const vernLang = useAppSelector(
(state: StoreState) =>
state.currentProjectState.project.vernacularWritingSystem.bcp47
Expand Down Expand Up @@ -276,7 +279,7 @@ export default function EditDialog(props: EditDialogProps): ReactElement {
}

// Remove empty/deleted senses; confirm nonempty vernacular and senses
const cleanedWord = cleanWord(newWord, true);
const cleanedWord = cleanWord(newWord, definitionsEnabled, true);
if (typeof cleanedWord === "string") {
toast.error(t(cleanedWord));
return Promise.reject(t(cleanedWord));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,10 @@ export default function EditSenseDialog(
(state: StoreState) =>
state.currentProjectState.project.analysisWritingSystems
);
const showDefinitions = useAppSelector(
const definitionsEnabled = useAppSelector(
(state: StoreState) => state.currentProjectState.project.definitionsEnabled
);
const showGrammaticalInfo = useAppSelector(
const grammaticalInfoEnabled = useAppSelector(
(state: StoreState) =>
state.currentProjectState.project.grammaticalInfoEnabled
);
Expand Down Expand Up @@ -138,7 +138,7 @@ export default function EditSenseDialog(
}

// Confirm nonempty senses
const cleanedSense = cleanSense(newSense);
const cleanedSense = cleanSense(newSense, definitionsEnabled);
if (!cleanedSense || typeof cleanedSense === "string") {
toast.error(t(cleanedSense ?? ""));
return;
Expand Down Expand Up @@ -205,7 +205,7 @@ export default function EditSenseDialog(
spacing={3}
>
{/* Definitions */}
{showDefinitions && (
{definitionsEnabled && (
<Grid item>
<Card sx={bgStyle(EditSenseField.Definitions)}>
<CardHeader title={t("reviewEntries.columns.definitions")} />
Expand Down Expand Up @@ -239,7 +239,7 @@ export default function EditSenseDialog(
</Grid>

{/* Part of Speech */}
{showGrammaticalInfo && (
{grammaticalInfoEnabled && (
<Grid item>
<Card sx={bgStyle(EditSenseField.GrammaticalInfo)}>
<CardHeader title={t("reviewEntries.columns.partOfSpeech")} />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -210,15 +210,31 @@ describe("cleanSense", () => {
sense.guid = "guid-does-not-matter";
sense.accessibility = Status.Protected;
expect(cleanSense(sense)).toBeUndefined();
expect(typeof cleanSense(sense, true)).toEqual("object");
expect(typeof cleanSense(sense, false, true)).toEqual("object");
});

it("returns error string for non-empty sense without gloss (unless protected and exempted)", () => {
const sense = newSense();
sense.grammaticalInfo.catGroup = GramCatGroup.Noun;
sense.accessibility = Status.Protected;
expect(typeof cleanSense(sense)).toEqual("string");
expect(typeof cleanSense(sense, true)).toEqual("object");
expect(typeof cleanSense(sense, false, true)).toEqual("object");
});

it("allow sense without gloss but with definition", () => {
const sense = newSense();
sense.definitions.push(newDefinition("¡Hola, Mundo!"));
expect(typeof cleanSense(sense)).toEqual("object");
});

it("returns different error if definitionsEnabled", () => {
const sense = newSense();
sense.grammaticalInfo.catGroup = GramCatGroup.Noun;
const disabledResult = cleanSense(sense, false);
const enabledResult = cleanSense(sense, true);
expect(typeof disabledResult).toEqual("string");
expect(typeof enabledResult).toEqual("string");
expect(disabledResult).not.toEqual(enabledResult);
});
});

Expand All @@ -242,7 +258,7 @@ describe("cleanWord", () => {
sense.accessibility = Status.Protected;
word.senses.push(sense);
expect(typeof cleanWord(word)).toEqual("string");
expect(typeof cleanWord(word, true)).toEqual("object");
expect(typeof cleanWord(word, false, true)).toEqual("object");
});

it("cleans up note", () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,11 @@ export function isSenseChanged(oldSense: Sense, newSense: Sense): boolean {
* - If a sense is marked as deleted or is utterly blank, return undefined
* - If a sense lacks gloss, return error string
*
* (If `definitionsEnabled = true`, can have definitions instead of glosses.)
* (If `exemptProtected = true`, protected senses are allowed to be without gloss.) */
export function cleanSense(
newSense: Sense,
definitionsEnabled = false,
exemptProtected = false
): Sense | string | undefined {
// Ignore deleted senses.
Expand Down Expand Up @@ -110,9 +112,11 @@ export function cleanSense(
return;
}

// Don't allow senses without a gloss.
if (!newSense.glosses.length) {
return "reviewEntries.error.gloss";
// Don't allow senses without a gloss or definition.
if (!newSense.glosses.length && !newSense.definitions.length) {
return definitionsEnabled
? "reviewEntries.error.glossAndDefinition"
: "reviewEntries.error.gloss";
}

return newSense;
Expand All @@ -122,8 +126,13 @@ export function cleanSense(
* - the vernacular field is empty
* - all senses are empty/deleted
*
* (If `definitionsEnabled = true`, can have definitions instead of glosses.)
* (If `exemptProtected = true`, protected senses are allowed to be empty.) */
export function cleanWord(word: Word, exemptProtected = false): Word | string {
export function cleanWord(
word: Word,
definitionsEnabled = false,
exemptProtected = false
): Word | string {
// Make sure vernacular isn't empty.
const vernacular = word.vernacular.trim();
if (!vernacular.length) {
Expand All @@ -133,7 +142,7 @@ export function cleanWord(word: Word, exemptProtected = false): Word | string {
// Clean senses and check for problems.
const senses: Sense[] = [];
for (const sense of word.senses) {
const cleanedSense = cleanSense(sense, exemptProtected);
const cleanedSense = cleanSense(sense, definitionsEnabled, exemptProtected);
// Skip deleted or empty senses.
if (!cleanedSense) {
continue;
Expand Down
Loading