Feature #769: Show error on duplicate property name (case insensitive) #1075
Conversation
There was a problem hiding this comment.
I'm not sure why GitHub is showing this in the diff. I think we merged this change already in a separate PR.
There was a problem hiding this comment.
Maybe a rebase would be useful? I'm now seeing that there's a merge conflict.
src/locales/en.json5
Outdated
| "warning": "Warning" | ||
| "warning": "Warning", | ||
| // Part of the error message format that lists the duplicate property name like "<PropertyName> (existing: <ExistingPropertyName>)" | ||
| "existing": "existing" |
There was a problem hiding this comment.
Some languages will use different punctuation from ( and ) and/or won't include a space character. Instead of translating "existing" specifically, I think we should translate the entire string, something like:
"{provided} (existing: {current})"If it includes the entire string, I think we should also change the message key. What do you think about entity.existingProperty or entity.duplicateProperty? Or maybe even better, you could change util.request.problem.409_17 from a single message to a group of messages that includes this one.
By the way, I think it makes sense not to include the bullet point in the string.
src/components/form/new.vue
Outdated
| @@ -224,6 +224,9 @@ export default { | |||
| actual: details.value | |||
| }); | |||
| } | |||
| if (code === 409.17) { | |||
| return `${this.$tcn('util.request.problem.409_17', details.duplicateProperties.length)}\n\n${details.duplicateProperties.map(p => `• ${p.provided} (${this.$t('common.existing')}: ${p.current})`).join('\n')}`; | |||
There was a problem hiding this comment.
It looks like this is the same in FormNew and FormPublish. If a 409.17 will always be handled in the same way, I think we can go ahead and move that logic to a centralized place in requestAlertMessage(). We do something similar for 404.1 Problems currently.
There was a problem hiding this comment.
In addition to FormNew and FormPublish, isn't this Problem a possibility for dataset/overview/new-property.vue? That could be another reason to centralize the handling of the Problem.
UPDATE: Never mind, I see now that it's a different Problem in that case.
sadiqkhoja
force-pushed
the
features/769_reject_duplicate_properties_ci
branch
from
ba93a03 to
1b114b9
Compare
src/locales/en.json5
Outdated
| "404_1": "The resource you are looking for cannot be found. The resource may have been deleted." | ||
| "404_1": "The resource you are looking for cannot be found. The resource may have been deleted.", | ||
| "409_17": { | ||
| "message": "This form attempts to create a new Entity property that matches with an existing one except for capitalization: | This form attempts to create new Entity properties that match with existing ones except for capitalization:", |
There was a problem hiding this comment.
| "message": "This form attempts to create a new Entity property that matches with an existing one except for capitalization: | This form attempts to create new Entity properties that match with existing ones except for capitalization:", | |
| "message": "This Form attempts to create a new Entity property that matches with an existing one except for capitalization: | This Form attempts to create new Entity properties that match with existing ones except for capitalization:", |
matthew-white
changed the title
sadiqkhoja
force-pushed
the
features/769_reject_duplicate_properties_ci
branch
from
0c79de7 to
96738d7
Compare
sadiqkhoja
force-pushed
the
features/769_reject_duplicate_properties_ci
branch
from
96738d7 to
1628490
Compare
Closes getodk/central#769
What has been done to verify that this works as intended?
Added tests and manual verification.
Why is this the best possible solution? Were any other approaches considered?
Uses existing approach of using
problemToAlertto capture and translate the error.Passing list of property names with bullet points to the
alertmethod doesn't look super clean to me but does the job. I am open to ideas to improve that.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?
None.
Does this change require updates to user documentation? If so, please file an issue here and include the link below.
getodk/docs#1855
Before submitting this PR, please make sure you have:
npm run testandnpm run lintand confirmed all checks still pass OR confirm CircleCI build passes