Conversation
Severino
left a comment
Severino
left a comment
There was a problem hiding this comment.
Required is not checked in the backend. Making the required option only a fronend gimick.
I would assume the backend has to verify that the required options are met.
| const attribute = state.requiredAttributes[attributeId]; | ||
|
|
||
| // check if existing value has an empty dirty value | ||
| // -> delete operation | ||
| if( | ||
| state.entity.data[attribute.id]?.id | ||
| && | ||
| hasKey(dirtyValues, attribute.id) | ||
| && | ||
| isEmpty(dirtyValues[attribute.id]) | ||
| ) { | ||
| isValid = false; | ||
| break; | ||
| } | ||
| // check if there is neither an existing valur nor: | ||
| // a) an existing entry in dirty values | ||
| // b) or the dirty value is empty | ||
| // -> missing | ||
| if( | ||
| !state.entity.data[attribute.id]?.id | ||
| && | ||
| ( | ||
| !hasKey(dirtyValues, attribute.id) | ||
| || | ||
| isEmpty(dirtyValues[attribute.id]) | ||
| ) | ||
| ) { | ||
| isValid = false; | ||
| break; | ||
| } |
There was a problem hiding this comment.
Move to meaningful helper file.
There was a problem hiding this comment.
This heavily relies on live data. Move this to a helper file with a lot of parameters feels wrong.
Maybe adding local functions (hasRequiredValueRemoved, hasRequiredValueEmpty) is a better solution?
resources/sass/_style.scss
Outdated
| height: 1.8rem; | ||
| width: 1.8rem; | ||
| background-color: $white; | ||
| /* background-color: $white; */ |
| <span | ||
| v-if="element.pivot?.metadata?.required" | ||
| class="text-danger" | ||
| > | ||
| <i class="fas fa-fw fa-xs fa-asterisk align-top" /> | ||
| </span> |
There was a problem hiding this comment.
This gets pushed underneath the attribute and is therefore not visible on long labels (that get truncated).
Good point 😁 😅 |
|
@Severino can we merge this? |
Severino
left a comment
Severino
left a comment
There was a problem hiding this comment.
It's not working for me.
I found the cause and other smaller issues.
| padding: 0; | ||
| height: 1.8rem; | ||
| width: 1.8rem; | ||
| background-color: $white; |
There was a problem hiding this comment.
This was necessary, otherwise the fab icons inside the data model editor's attribute list will show the underlying text. Either reenable or give the fab-button group a white bg as in the entity type list.
| let isValid = true; | ||
| const dirtyValues = getDirtyValues(grps); | ||
|
|
||
| for(let attributeId in state.requiredAttributes) { |
There was a problem hiding this comment.
This is incorrect. Here you receive the array index of the attributes not the id, should be:
for(let attribute of state.requiredAttributes) There was a problem hiding this comment.
Is this really a problem? As far as I see is the only problem that that variable is attributeId instead of something like requiredArrayIndex. I only reference attributeId to get the actual attribute (const attribute = state.requiredAttributes[attributeId]). From that on I only reference attribute.id. This should be the same as your approach.
Nevertheless is the variable name confusing.
There was a problem hiding this comment.
As you say. It's not really a problem during execution, but the name is still incorrect.
It didn't work for me at all while testing and the wrong naming required additional debugging until I realized that it's not the attributeId I'm working with. Just JavaScript things :)
There was a problem hiding this comment.
I switched to your for … of approach, because the index is not important and direct access to attribute is much easier.
| const isFormValid = (grps, asToast = true) => { | ||
| let isValid = true; | ||
| const dirtyValues = getDirtyValues(grps); | ||
|
|
||
| for(let attributeId in state.requiredAttributes) { | ||
| const attribute = state.requiredAttributes[attributeId]; | ||
|
|
||
| // check if existing value has an empty dirty value | ||
| // -> delete operation | ||
| if( | ||
| state.entity.data[attribute.id]?.id | ||
| && | ||
| hasKey(dirtyValues, attribute.id) | ||
| && | ||
| isEmpty(dirtyValues[attribute.id]) | ||
| ) { | ||
| isValid = false; | ||
| break; | ||
| } | ||
| // check if there is neither an existing valur nor: | ||
| // a) an existing entry in dirty values | ||
| // b) or the dirty value is empty | ||
| // -> missing | ||
| if( | ||
| !state.entity.data[attribute.id]?.id | ||
| && | ||
| ( | ||
| !hasKey(dirtyValues, attribute.id) | ||
| || | ||
| isEmpty(dirtyValues[attribute.id]) | ||
| ) | ||
| ) { | ||
| isValid = false; | ||
| break; | ||
| } |
There was a problem hiding this comment.
This seems unnecessarily complicated, why can't we just use this:
for(let attribute of state.requiredAttributes) {
// If the value is modified we need to check the dirty value, otherwise we need to check the existing value
const dirtyOrExistingValue = (dirtyValues[attribute.id]) ? dirtyValues[attribute.id] : state.entity.data[attribute.id]?.value;
if(isEmpty(dirtyOrExistingValue)) {
isValid = false;
break;
}
}There was a problem hiding this comment.
I have to re-check. But if I remember correctly, it needs these two different checks, because of all the different states.
There was a problem hiding this comment.
Your ternary condition is wrong.
For the first (value deleted) check we explicitly have to check for existing value in the entity data and an existing, but empty value in dirty values. Your code falls back to the existing value and thus allow deleting data
The second check seems to be identical from the logic, but I prefer the "verbose" check so it is obvious.
There was a problem hiding this comment.
Yeah, you are right. But still I would prefer a more readable solution. What really bugged me was the
if(id & ....)
if(!id & ....)
Which suggests that it's more an if/else situation. But working on it I also used a different approach.
for(let attribute of state.requiredAttributes) {
const dataEntryExists = Boolean(state.entity.data[attribute.id]?.id);
const dirtyRequiredValue = dirtyValues[attribute.id];
const deletingRequired = dataEntryExists && hasKey(dirtyValues, attribute.id) && isEmpty(dirtyRequiredValue);
const requiredNotSet = !dataEntryExists && hasKey(dirtyValues, attribute.id) && !isEmpty(dirtyRequiredValue);
if(deletingRequired || requiredNotSet)
{
isValid = false;
break;
}
}| const onRequireEntityAttribute = e => { | ||
| const attribute = state.entityAttributes.find(attribute => { | ||
| return attribute.id == e.element.id; | ||
| }); | ||
| if(attribute) { | ||
| const isRequired = e.active === true; | ||
| const metadata = { | ||
| required: isRequired, | ||
| }; | ||
| const attributeId = e.element.id; | ||
| const entityAttributeId = e.element.pivot.id; | ||
| entityStore.patchEntityMetadata( | ||
| state.entityType.id, | ||
| attributeId, | ||
| entityAttributeId, | ||
| metadata, | ||
| ).then(_ => { | ||
| if(!attribute.pivot.metadata) { | ||
| attribute.pivot.metadata = {}; | ||
| } | ||
| attribute.pivot.metadata.required = isRequired; | ||
| }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Can't we just handle this via the store?
Normally the true value is inside the store, then we shouldn't have to update a value inside a prop.
Imo, it should be as easy as:
entityAttributeStore.setRequired(element.id, required)
Additionally:
It could also be executed directly on the attribute list.
As the fab controls will forever have this single purpose.
Those could be put into a slot of the AttributeList, but separating those from this
logic does not make much sense.
2 participants
This PR adds the possibility to flag entity attributes as required. This require users to fill in required attributes before saving an entity.
I added
isFormValid()to EntityDetail.vue as function, because it didn't work as computed prop. It has an optional parameter (default is true) to show a toast, if it fails because of missing required attributes.