Enhancement: Add validation for unique values for a short text fields #1770

Koc · 2025-04-24T18:34:45Z

This PR adds new toggle that validates short text fields for unique values. Closes #660

🔍 Preview

Trying to insert row with value that already present in table

blizzz

Didn't test yet, so far looks good, left minor requests.

However, existing tests seem to be failing. Having additional tests, integration, e2e or both would be appreciated.

lib/Migration/Version000920Date20250422000000.php

lib/Service/RowService.php

Koc · 2025-05-10T15:43:21Z

@blizzz I've updated PR and 1st message including screenshoots. Now we have user-friendly error message with an explanation why row wasn't added. Old tests should pass.

Also I'm trying to add new integration tests here. If anybody can help me with that - welcome

Koc · 2025-05-10T23:09:31Z

@blizzz Old tests fixed. Also there is 1 integration test for a new functionality

blizzz

A few questions.

I did a little bit of smoke testing, and it looks good so far 👍

Three more points I like to raise from this review:

There is one corner case though: when you have a column with non-unique values, but edit the column and set it to unique, then it works. But obviously the content there is not unique, but new values will be checked for it.

Is this intentional behaviour? An alternative would be to reject the change with the error that the column values are not unique.

Another thought came up about the return status code. You set it to 400, which is OK, I am thinking 409 could maybe be more precise. But maybe this is also bike shedding 🤔
When you try to update a row to have a duplicated value, the operations fails , but with status code 500 instead of 400. This should be sorted out.

I hope I've covered all points now 🙇 Thank you for the work on this feature!

cypress/fixtures/widgets/richObject.json

blizzz · 2025-05-12T18:44:40Z

lib/Controller/AOCSController.php

-		return new DataResponse(['message' => $this->n->t('An error caused by an invalid request occurred. More details can be found in the logs. Please reach out to your administration.')], Http::STATUS_BAD_REQUEST);
+		return new DataResponse(['message' => $e->getMessage()], Http::STATUS_BAD_REQUEST);


Why do we remove the translated error string?

Because now we're providing not generic error message but more self-explanatory Column "My unique column" contains not unique value.

That's true, but the exception message is not translated either. They should not be translated for the log, but user facing messages should be translated (they can be specific of course, that is an improvement 🙂).

I've refactored it a bit and added optional translatedMessage property to the BadRequestError exception.

appinfo/info.xml

lib/Controller/ContextController.php

tests/integration/features/APIv2.feature

lib/Service/ColumnTypes/TextLineBusiness.php

Koc · 2025-05-13T07:59:39Z

@blizzz thank you for code review and deep testing 🫶 . I'm going to fix your finding later today

Koc · 2025-05-14T15:32:39Z

hey @blizzz!

But obviously the content there is not unique, but new values will be checked for it.

Yeah, this is expected behavior. We can improve it later, but as you can see - current PR already huge.

Let's keep 400 for now and think about changing it later? It can be http 409 or 422
I've improved editing column with non-unique value:

Now we have 1 single clear message. Previously we have 2 abstract messages that not help at all

🔍 Preview
We don't close popup window in order to not loose possible unsaved changes
Http response has code 400 instead of 500
Changes covered with behat tests

Koc · 2025-05-20T22:33:15Z

Can we merge @blizzz @enjeck ? 🙏

enjeck · 2025-05-21T05:08:22Z

Can we merge @blizzz @enjeck ? 🙏

It's unrelated to this, but I'd prefer to hold off on any merges until some cypress issues are fixed. Right now, Cypress tests fail everywhere and I'd like to fix that first, just to be safe

blizzz · 2025-05-23T09:10:40Z

Can we merge @blizzz @enjeck ? 🙏

It's unrelated to this, but I'd prefer to hold off on any merges until some cypress issues are fixed. Right now, Cypress tests fail everywhere and I'd like to fix that first, just to be safe

Cypress one`s should hopefully be fixed now with #1809 – @Koc could you rebase?

Koc · 2025-05-23T23:10:30Z

rebased. Almost green

Koc · 2025-05-25T11:59:57Z

seems like we have same error in other PRs: https://github.com/nextcloud/tables/actions/runs/15216779497/job/42804024347

Koc · 2025-05-27T13:28:20Z

So, we have completely green pipeline including Cypress @blizzz @enjeck

blizzz

please see comments

lib/Service/ColumnTypes/TextLineBusiness.php

blizzz · 2025-06-10T17:25:25Z

lib/Controller/AOCSController.php

-		return new DataResponse(['message' => $this->n->t('An error caused by an invalid request occurred. More details can be found in the logs. Please reach out to your administration.')], Http::STATUS_BAD_REQUEST);
+		return new DataResponse(['message' => $e->getMessage()], Http::STATUS_BAD_REQUEST);


That's true, but the exception message is not translated either. They should not be translated for the log, but user facing messages should be translated (they can be specific of course, that is an improvement 🙂).

cypress/e2e/tables-table.cy.js

Signed-off-by: Kostiantyn Miakshyn <[email protected]>

…code review fixes) Signed-off-by: Kostiantyn Miakshyn <[email protected]>

Koc requested review from enjeck and blizzz as code owners April 24, 2025 18:34

Koc force-pushed the feature/add-unique-values branch from 4bcc937 to 8f0684a Compare April 24, 2025 22:39

blizzz reviewed Apr 28, 2025

View reviewed changes

lib/Migration/Version000920Date20250422000000.php Show resolved Hide resolved

lib/Service/RowService.php Outdated Show resolved Hide resolved

blizzz added enhancement New feature or request 2. developing Work in progress labels Apr 28, 2025

Koc force-pushed the feature/add-unique-values branch from 8f0684a to 12a33b3 Compare April 30, 2025 11:01

Koc force-pushed the feature/add-unique-values branch 4 times, most recently from 276e95d to 7a20c4b Compare May 10, 2025 15:41

Koc force-pushed the feature/add-unique-values branch 4 times, most recently from a3d3049 to adf32d1 Compare May 10, 2025 22:56

blizzz reviewed May 12, 2025

View reviewed changes

Koc force-pushed the feature/add-unique-values branch 6 times, most recently from 36f6088 to 52d3fab Compare May 14, 2025 15:29

Koc force-pushed the feature/add-unique-values branch 3 times, most recently from 35cd347 to 0f62241 Compare May 14, 2025 15:49

Koc force-pushed the feature/add-unique-values branch from 0f62241 to 3a9c5bc Compare May 23, 2025 22:14

Koc force-pushed the feature/add-unique-values branch from 3a9c5bc to 4cf6dca Compare May 27, 2025 12:41

blizzz reviewed Jun 10, 2025

View reviewed changes

Enhancement: Add validation for unique values for short text fields

8d08aed

Signed-off-by: Kostiantyn Miakshyn <[email protected]>

Koc force-pushed the feature/add-unique-values branch 2 times, most recently from d49fd28 to 26a11d8 Compare June 23, 2025 16:04

Enhancement: Add validation for unique values for short text fields (…

119e66b

…code review fixes) Signed-off-by: Kostiantyn Miakshyn <[email protected]>

Koc force-pushed the feature/add-unique-values branch from 26a11d8 to 119e66b Compare June 23, 2025 22:19

		return new DataResponse(['message' => $this->n->t('An error caused by an invalid request occurred. More details can be found in the logs. Please reach out to your administration.')], Http::STATUS_BAD_REQUEST);
		return new DataResponse(['message' => $e->getMessage()], Http::STATUS_BAD_REQUEST);

Enhancement: Add validation for unique values for a short text fields #1770

Are you sure you want to change the base?

Enhancement: Add validation for unique values for a short text fields #1770

Uh oh!

Conversation

Koc commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blizzz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Koc commented May 10, 2025

Uh oh!

Koc commented May 10, 2025

Uh oh!

blizzz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

blizzz May 12, 2025

Choose a reason for hiding this comment

Uh oh!

Koc May 13, 2025

Choose a reason for hiding this comment

Uh oh!

Koc May 13, 2025

Choose a reason for hiding this comment

Uh oh!

blizzz Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

Koc Jun 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Koc commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Koc commented May 14, 2025

Uh oh!

Koc commented May 20, 2025

Uh oh!

enjeck commented May 21, 2025

Uh oh!

blizzz commented May 23, 2025

Uh oh!

Koc commented May 23, 2025

Uh oh!

Koc commented May 25, 2025

Uh oh!

Koc commented May 27, 2025

Uh oh!

blizzz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

blizzz Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Koc commented Apr 24, 2025 •

edited

Loading

Koc commented May 13, 2025 •

edited

Loading