Skip to content

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

Merged
blizzz merged 2 commits into
mainfrom
feature/add-unique-values
Aug 15, 2025
Merged

Enhancement: Add validation for unique values for a short text fields#1770
blizzz merged 2 commits into
mainfrom
feature/add-unique-values

Conversation

@Koc

@Koc Koc commented Apr 24, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

🔍 Preview

image

Trying to insert row with value that already present in table
image

@Koc Koc requested review from blizzz and enjeck as code owners April 24, 2025 18:34
@Koc Koc force-pushed the feature/add-unique-values branch from 4bcc937 to 8f0684a Compare April 24, 2025 22:39
blizzz
blizzz reviewed Apr 28, 2025
View reviewed changes

@blizzz blizzz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment thread lib/Migration/Version000920Date20250422000000.php
Comment thread lib/Service/RowService.php Outdated
@blizzz blizzz added enhancement New feature or request 2. developing Work in progress labels Apr 28, 2025
@Koc Koc force-pushed the feature/add-unique-values branch from 8f0684a to 12a33b3 Compare April 30, 2025 11:01
@Koc Koc force-pushed the feature/add-unique-values branch 4 times, most recently from 276e95d to 7a20c4b Compare May 10, 2025 15:41
@Koc

Koc commented May 10, 2025

Copy link
Copy Markdown
Contributor Author

@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 Koc force-pushed the feature/add-unique-values branch 4 times, most recently from a3d3049 to adf32d1 Compare May 10, 2025 22:56
@Koc

Koc commented May 10, 2025

Copy link
Copy Markdown
Contributor Author

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

blizzz
blizzz reviewed May 12, 2025
View reviewed changes

@blizzz blizzz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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:

  1. 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.

  1. 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 🤔

  2. 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!

Comment thread cypress/fixtures/widgets/richObject.json Outdated
Comment thread lib/Controller/AOCSController.php Outdated
Comment thread appinfo/info.xml Outdated
Comment thread lib/Controller/ContextController.php
Comment thread tests/integration/features/APIv2.feature Outdated
Comment thread lib/Service/ColumnTypes/TextLineBusiness.php Outdated
@Koc

Koc commented May 13, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

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

Koc commented May 14, 2025

Copy link
Copy Markdown
Contributor Author

hey @blizzz!

  1. 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.

  1. Let's keep 400 for now and think about changing it later? It can be http 409 or 422

  2. 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

    image

  • 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 Koc force-pushed the feature/add-unique-values branch 3 times, most recently from 35cd347 to 0f62241 Compare May 14, 2025 15:49
@Koc

Koc commented May 20, 2025

Copy link
Copy Markdown
Contributor Author

Can we merge @blizzz @enjeck ? 🙏

@enjeck

enjeck commented May 21, 2025

Copy link
Copy Markdown
Contributor

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

blizzz commented May 23, 2025

Copy link
Copy Markdown
Member

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 Koc force-pushed the feature/add-unique-values branch from 0f62241 to 3a9c5bc Compare May 23, 2025 22:14
@Koc

Koc commented May 23, 2025

Copy link
Copy Markdown
Contributor Author

rebased. Almost green

@Koc

Koc commented May 25, 2025

Copy link
Copy Markdown
Contributor Author

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

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

Koc commented May 27, 2025

Copy link
Copy Markdown
Contributor Author

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

blizzz
blizzz reviewed Jun 10, 2025
View reviewed changes

@blizzz blizzz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

please see comments

Comment thread lib/Service/ColumnTypes/TextLineBusiness.php Outdated
Comment thread lib/Controller/AOCSController.php Outdated
Comment thread cypress/e2e/tables-table.cy.js Outdated
@Koc Koc force-pushed the feature/add-unique-values branch 3 times, most recently from 26a11d8 to 119e66b Compare June 23, 2025 22:19
@Koc Koc force-pushed the feature/add-unique-values branch from 119e66b to 6822509 Compare June 30, 2025 23:14
@Koc

Koc commented Jul 1, 2025

Copy link
Copy Markdown
Contributor Author

@blizzz code review comments fixed

@Koc Koc force-pushed the feature/add-unique-values branch 4 times, most recently from 17dcf17 to b90b1a5 Compare July 8, 2025 23:03
@Koc Koc force-pushed the feature/add-unique-values branch 3 times, most recently from 00ba5eb to 8472363 Compare July 18, 2025 22:24
@enjeck

enjeck commented Jul 24, 2025

Copy link
Copy Markdown
Contributor

@blizzz Good to merge?

Koc added 2 commits August 3, 2025 19:29
@Koc
Enhancement: Add validation for unique values for short text fields
e5c7720 
Signed-off-by: Kostiantyn Miakshyn <molodchick@gmail.com>
@Koc
Enhancement: Improve filtering by groups and teams (try fix)
5cfb26a 
Signed-off-by: Kostiantyn Miakshyn <molodchick@gmail.com>
@Koc Koc force-pushed the feature/add-unique-values branch from 8472363 to 5cfb26a Compare August 3, 2025 16:29
@Koc Koc requested a review from blizzz August 14, 2025 12:20
@blizzz blizzz merged commit 0c481f3 into main Aug 15, 2025
64 checks passed
@blizzz blizzz deleted the feature/add-unique-values branch August 15, 2025 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@blizzz blizzz blizzz approved these changes

@enjeck enjeck Awaiting requested review from enjeck enjeck is a code owner

Assignees

No one assigned

Labels

2. developing Work in progress enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

UNIQUE constraint on columns

3 participants

@Koc @enjeck @blizzz