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

pkp/pkp-lib#9996 Initial attempt to ignore hidden fields for validate… #357

Merged
merged 4 commits into from
Jun 13, 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
6 changes: 5 additions & 1 deletion src/components/Form/Form.vue
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
<script>
import FormLocales from './FormLocales.vue';
import FormPage from './FormPage.vue';
import {shouldShowField} from './formHelpers';

export default {
name: 'PkpForm',
Expand Down Expand Up @@ -332,7 +333,10 @@ export default {
validateRequired() {
let errors = {};
this.fields.forEach((field) => {
if (!field.isRequired) {
if (
!field.isRequired ||
!shouldShowField(field, this.fields, this.groups)
) {
return;
}
let missingValue = false;
Expand Down
46 changes: 19 additions & 27 deletions src/components/Form/FormGroup.vue
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@ import FieldUpload from './fields/FieldUpload.vue';
import FieldSlider from './fields/FieldSlider.vue';
import FieldUploadImage from './fields/FieldUploadImage.vue';

import {shouldShowFieldWithinGroup} from './formHelpers';

export default {
name: 'FormGroup',
components: {
Expand Down Expand Up @@ -127,7 +129,9 @@ export default {
*/
fieldsInGroup() {
return this.fields.filter(
(field) => field.groupId === this.id && this.shouldShowField(field),
(field) =>
field.groupId === this.id &&
shouldShowFieldWithinGroup(field, this.fields),
);
},

Expand All @@ -153,30 +157,6 @@ export default {
this.$emit('change', name, prop, value, localeKey);
},

/**
* Should a field be shown?
*
* @param {Object} field One of this.fields
* @return {Boolean}
*/
shouldShowField: function (field) {
if (typeof field.showWhen === 'undefined') {
return true;
}
const whenFieldName =
typeof field.showWhen === 'string' ? field.showWhen : field.showWhen[0];
const whenField = this.fields.find(
(field) => field.name === whenFieldName,
);
if (!whenField) {
return false;
}
if (typeof field.showWhen === 'string') {
return !!whenField.value;
}
return whenField.value === field.showWhen[1];
},

/**
* Respond to a field changing its errors
*
Expand All @@ -185,7 +165,19 @@ export default {
* @param {String} localeKey The locale key for a multilingual field
*/
setFieldErrors: function (name, errors, localeKey = '') {
let newErrors = {...this.errors};
/**
* Better practice is to make to copy of data, modify it and send it upstream
* But there is challenge with #9996, where each unmounted fields needs to clean up
* its error. And given its happening synchronously, it does not propagate updated errors
* object in between the unmounts. Therefore last operation would override the previous ones.
*
* Changing error object inplace ensures all the updates gets registered.
*
* In future better approach would be to have remove-error event, which defines what to remove
* and would be correctly processed by the state manager
*/

let newErrors = this.errors;
if (!errors || !errors.length) {
if (localeKey && newErrors[name] && newErrors[name][localeKey]) {
delete newErrors[name][localeKey];
Expand All @@ -200,7 +192,7 @@ export default {
newErrors[name] = errors;
}
}
this.$emit('set-errors', newErrors);
this.$emit('set-errors', {...newErrors});
},
},
};
Expand Down
29 changes: 3 additions & 26 deletions src/components/Form/FormPage.vue
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@
import ButtonRow from '@/components/ButtonRow/ButtonRow.vue';
import FormErrors from '@/components/Form/FormErrors.vue';
import FormGroup from '@/components/Form/FormGroup.vue';

import {shouldShowGroup} from './formHelpers';
export default {
name: 'FormPage',
components: {
Expand Down Expand Up @@ -107,7 +107,8 @@ export default {
*/
groupsInPage() {
return this.groups.filter(
(group) => group.pageId === this.id && this.shouldShowGroup(group),
(group) =>
group.pageId === this.id && shouldShowGroup(group, this.fields),
);
},

Expand Down Expand Up @@ -186,30 +187,6 @@ export default {
this.$emit('showLocale', localeKey);
},

/**
* Should a group be shown?
*
* @param {Object} group One of this.groups
* @return {Boolean}
*/
shouldShowGroup: function (group) {
if (typeof group.showWhen === 'undefined') {
return true;
}
const whenFieldName =
typeof group.showWhen === 'string' ? group.showWhen : group.showWhen[0];
const whenField = this.fields.find(
(field) => field.name === whenFieldName,
);
if (!whenField) {
return false;
}
if (typeof group.showWhen === 'string') {
return !!whenField.value;
}
return whenField.value === group.showWhen[1];
},

/**
* Pass an event up to the form to set the errors object
*
Expand Down
4 changes: 4 additions & 0 deletions src/components/Form/fields/FieldBase.vue
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ export default {
emits: [
/** Emitted when a field prop changes. Payload: `(fieldName, propName, newValue, [localeKey])`. The `localeKey` will be null for fields that are not multilingual. This event is fired every time the `value` changes, so you should [debounce](https://www.npmjs.com/package/debounce) event callbacks that contain resource-intensive code. */
'change',
'set-errors',
],
computed: {
/**
Expand Down Expand Up @@ -255,6 +256,9 @@ export default {
});
},
},
beforeUnmount() {
this.$emit('set-errors', this.name, [], this.localeKey);
},
methods: {
/**
* Helper function to compile unique IDs for labels and aria-describedby
Expand Down
59 changes: 59 additions & 0 deletions src/components/Form/formHelpers.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/**
* Should a field be shown?
*
* @param {Object} field One of this.fields
* @return {Boolean}
*/
export function shouldShowFieldWithinGroup(field, allFields) {
Copy link
Contributor

Choose a reason for hiding this comment

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

allFields doesn't have a JSDoc comment above.

if (typeof field.showWhen === 'undefined') {
return true;
}
const whenFieldName =
typeof field.showWhen === 'string' ? field.showWhen : field.showWhen[0];
const whenField = allFields.find((field) => field.name === whenFieldName);
if (!whenField) {
return false;
}
if (typeof field.showWhen === 'string') {
return !!whenField.value;
}
return whenField.value === field.showWhen[1];
}
/**
* Should a group be shown?
*
* @param {Object} group One of this.groups
* @return {Boolean}
*/
export function shouldShowGroup(group, fields) {
Copy link
Contributor

Choose a reason for hiding this comment

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

fields doesn't have a JSDoc comment above.

if (typeof group.showWhen === 'undefined') {
return true;
}
const whenFieldName =
typeof group.showWhen === 'string' ? group.showWhen : group.showWhen[0];
const whenField = fields.find((field) => field.name === whenFieldName);
if (!whenField) {
return false;
}
if (typeof group.showWhen === 'string') {
return !!whenField.value;
}
return whenField.value === group.showWhen[1];
}

/**
* Should a field be shown?
*
* @param {Object} field One field to check
* @param {Object} fields All form fields
* @param {Object} groups All form groups
* @return {Boolean}
*/

export function shouldShowField(field, fields, groups) {
const group = groups.find((group) => group.id === field.groupId);

return (
shouldShowGroup(group, fields) && shouldShowFieldWithinGroup(field, fields)
);
}