Don't require label in entity update and allow other blank updates #1035
Conversation
lib/data/entity.js
Outdated
| // that is ok -- the label just wont be updated on the entity. | ||
| // TODO: do something different with validateEntity because it has too | ||
| // many special, subtle cases. | ||
| if (!(update === '1' || update === 'true') && (!label || label.trim() === '')) |
There was a problem hiding this comment.
Is there any chance we could get parseSubmissionXml() to convert the create and update attributes to JavaScript booleans? I think the result would be more readable relative to the first "or" condition here involving update.
lib/data/entity.js
Outdated
| // that is ok -- the label just wont be updated on the entity. | ||
| // TODO: do something different with validateEntity because it has too | ||
| // many special, subtle cases. | ||
| if (!(update === '1' || update === 'true') && (!label || label.trim() === '')) |
There was a problem hiding this comment.
I'm thinking of getodk/central#514 and wondering whether we should adjust the logic here slightly. Rather than "if the update attribute is not true, then require a label," it could be "if the create attribute is true, then require a label, even if the update attribute is also true." We could also come back to this later.
There was a problem hiding this comment.
see totally new logic - instead of calling this validateEntity function that tries to do it all, while also manipulating the structure, the each field is pulled out and validated on its own to build up the new entity def.
ktuite
force-pushed
the
ktuite/entity_update_label
branch
from
302d51b
to
e3a7124
Compare
ktuite
force-pushed
the
ktuite/entity_update_label
branch
from
03f50ce
to
5759dfa
Compare
lib/data/entity.js
Outdated
| } | ||
|
|
||
| if (body.label != null) | ||
| if (typeof body.label !== 'string') | ||
| throw Problem.user.invalidDataTypeOfParameter({ field: 'label', expected: 'string' }); | ||
| else if (body.label.trim() === '') | ||
| throw Problem.user.missingParameter({ field: 'label' }); |
There was a problem hiding this comment.
Is there a different Problem that could be used here? The message of this Problem is "Required parameter label missing.", but the label isn't actually required. Maybe a 400.8 (unexpectedValue)?
| const { fieldsFor } = require(appRoot + '/test/util/schema'); | ||
| const testData = require(appRoot + '/test/data/xml'); | ||
|
|
||
| describe('extracting and validating entities', () => { | ||
| describe('validateEntity', () => { |
There was a problem hiding this comment.
Is there other test coverage for the cases here? For example, I'm having trouble finding a test that verifies that trying to create an entity via submission without a label results in an error. Similarly, I'm not seeing a test about the case where the baseVersion attribute isn't an integer. I could be missing them though.
ktuite
force-pushed
the
ktuite/entity_update_label
branch
from
2dcdf56
to
0dcb9c2
Compare
Closes getodk/central#527 by allowing an entity update submission with no
<label>tag specified.Specifying a blank
<label></label>will be treated as ignoring the label (but only during an update).Specifying an empty property
<age></age>can now be used to set that property to blank.The PR also adds a new Problem for not being able to find the base version of an entity, which should fix this one case of not seeing any error on frontend: getodk/central#536
What has been done to verify that this works as intended?
tests
should probably add tests for the new individual functions i added, but most of the cases are also tested through higher level tests.
Why is this the best possible solution? Were any other approaches considered?
This led me to refactor
validateEntity(). It used to be that there was some validation spread out through the code (e.g. checking the dataset first to know if it existed) and then this one validation function. Awkwardly, that function took 4 different kinds of input (submissions vs. api crossed with create vs. update) and it also slightly modified the object it was checking to "help" prepare data to put into an Entity frame. It wasn't really helping!Now the different fields are validated when they are pulled out because they are only needed in certain cases, e.g.
baseVersionis only relevant in an update from a submission, andlabelcan or can't be blank depending on the context. (on an update from a submission, if it is blank, it will be ignored, but in other cases, it will throw an error.)How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
Better entity update behavior, clearer errors.
Does this change require updates to the API documentation? If so, please update docs/api.md as part of this PR.
I don't think so.
Before submitting this PR, please make sure you have:
make test-fulland confirmed all checks still pass OR confirm CircleCI build passes