Skip to content

1769 views set mandatory state of a column - #2053

Merged
enjeck merged 22 commits into
mainfrom
1769-views-set-mandatory-state-of-a-column
Sep 25, 2025
Merged

1769 views set mandatory state of a column#2053
enjeck merged 22 commits into
mainfrom
1769-views-set-mandatory-state-of-a-column

Conversation

@silverkszlo

@silverkszlo silverkszlo commented Sep 10, 2025

Copy link
Copy Markdown
Contributor

Resolves #1769

Add the possibility to make columns in views mandatory.

TODO:

  • add tests
  • fix validation for mandatory datetime fields
Screenshot from 2025-09-15 12-15-31

@silverkszlo silverkszlo linked an issue Sep 10, 2025 that may be closed by this pull request
5 tasks
@silverkszlo
silverkszlo force-pushed the 1769-views-set-mandatory-state-of-a-column branch from 663a888 to ec28231 Compare September 11, 2025 16:07
@enjeck
enjeck force-pushed the 1769-views-set-mandatory-state-of-a-column branch from ec28231 to cb90299 Compare September 14, 2025 08:27
@enjeck
enjeck marked this pull request as ready for review September 14, 2025 10:22
@enjeck

enjeck commented Sep 14, 2025

Copy link
Copy Markdown
Contributor

@blizz Would appreciate a review!

Comment thread lib/Service/RowService.php Outdated
$defaultValue = $column->getSelectionDefault();
return $defaultValue !== null && $defaultValue !== '' && $defaultValue !== '[]';
}
return $value !== null && $value !== '';

@enjeck enjeck Sep 14, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Doing all this to ensure that when row requests are made via the API, the mandatory condition is respected. Maybe there's an easier way to do it?

@silverkszlo silverkszlo reopened this Sep 15, 2025
@silverkszlo
silverkszlo force-pushed the 1769-views-set-mandatory-state-of-a-column branch 5 times, most recently from 21da51b to 64dc43f Compare September 17, 2025 08:14
@blizzz

blizzz commented Sep 17, 2025

Copy link
Copy Markdown
Member

in ResponseDefinitions.php, the API documentation need to be extended by the mandatory key, similar as in https://github.com/nextcloud/tables/pull/1958/files#diff-b15c74adf6c164ed61707f0a48432bb4df4972b0f3994256778091bcd83172e8R29

@silverkszlo
silverkszlo force-pushed the 1769-views-set-mandatory-state-of-a-column branch from 7c99707 to 88d1be8 Compare September 17, 2025 13:05
@silverkszlo

Copy link
Copy Markdown
Contributor Author

@enjeck and @blizzz I would appreciate another review of the latest commits beginning with that one: e309b7e

@blizzz

blizzz commented Sep 17, 2025

Copy link
Copy Markdown
Member

Would appreciate a review!

The one with two z is some other person btw 😁 (classic)

@blizzz

blizzz commented Sep 17, 2025

Copy link
Copy Markdown
Member

Works like a charm!

I suppose the cypress errors are unrelated…?

@silverkszlo
silverkszlo force-pushed the 1769-views-set-mandatory-state-of-a-column branch from 817dd25 to 3346898 Compare September 18, 2025 15:02
@silverkszlo

Copy link
Copy Markdown
Contributor Author

Works like a charm!

I suppose the cypress errors are unrelated…?

The failing test in view-filtering-selection.cy.js was related to this PR, because I have fixed a typo in ViewSettings.vue (changed this.viewSettings to this.viewSetting in saveText()). Because of the typo the else if-clause would never come true and the button would never say "Save modified View". It always said "Save View" instead, which the test was expecting. Fixing the typo meant that the button would display "Save modified View" eventually, so I have adjusted to test and now it passes.

The failing test in view.cy.js on ther other hand is unrelated to this PR because it also fails in #1718 . As I have just noticed that now and thought that the failing test was indeed related to this PR, I have worked on a fix here. It seems that it did the trick. Had to restructure the test and follow the pattern of other passing tests.

  • The test was failing because it couldn't find the customTableAction button. The original test tried to access customTableAction in the beforeEach hook before properly loading the table context.
  • I also adjusted the Cleanup Strategy: Changed from afterEach() to after() to prevent table deletion between tests.
    This prevents row accumulation issues across tests.

The key insight was that customTableAction only renders properly when the table is loaded in the correct context via cy.loadTable(), not when accessed immediately after table creation.

Comment thread cypress/e2e/view.cy.js
Comment thread cypress/e2e/view.cy.js
Comment thread cypress/e2e/view-mandatory-state.cy.js
@silverkszlo
silverkszlo force-pushed the 1769-views-set-mandatory-state-of-a-column branch from 115a08a to 21e4814 Compare September 22, 2025 07:02
@enjeck
enjeck force-pushed the 1769-views-set-mandatory-state-of-a-column branch from 042e07d to 01af1be Compare September 24, 2025 05:14

@enjeck enjeck left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was just testing again and I realized that the mandatory field doesn't work for the SelectionCheckForm type. Both checked and unchecked cases give an error saying that the field is empty.

The UsergroupForm doesn't work either, allowing me to save even when empty and mandatory. THis is also a problem not just for the views, but for the mandatory field that can be set on the table's column (unrelated to this PR, but could be nice to fix here while fixing the view case)

@silverkszlo
silverkszlo force-pushed the 1769-views-set-mandatory-state-of-a-column branch from 01af1be to 4855de3 Compare September 24, 2025 11:30
silverkszlo and others added 22 commits September 25, 2025 04:54
Signed-off-by: silver <s.szmajduch@posteo.de>
Signed-off-by: silver <s.szmajduch@posteo.de>
Signed-off-by: silver <s.szmajduch@posteo.de>
Signed-off-by: silver <s.szmajduch@posteo.de>
Signed-off-by: silver <s.szmajduch@posteo.de>
Signed-off-by: Enjeck C <patrathewhiz@gmail.com>
Signed-off-by: Enjeck C <patrathewhiz@gmail.com>
Signed-off-by: silver <s.szmajduch@posteo.de>
Signed-off-by: silver <s.szmajduch@posteo.de>
Signed-off-by: silver <s.szmajduch@posteo.de>
Signed-off-by: silver <s.szmajduch@posteo.de>
…ettings

Signed-off-by: silver <s.szmajduch@posteo.de>
Signed-off-by: silver <s.szmajduch@posteo.de>
Signed-off-by: silver <s.szmajduch@posteo.de>
Signed-off-by: silver <s.szmajduch@posteo.de>
Signed-off-by: silver <s.szmajduch@posteo.de>
Signed-off-by: silver <s.szmajduch@posteo.de>
Signed-off-by: silver <s.szmajduch@posteo.de>
Signed-off-by: silver <s.szmajduch@posteo.de>
Signed-off-by: silver <s.szmajduch@posteo.de>
Signed-off-by: silver <s.szmajduch@posteo.de>
Signed-off-by: Enjeck C <patrathewhiz@gmail.com>
@enjeck
enjeck force-pushed the 1769-views-set-mandatory-state-of-a-column branch from 9d89c0e to 2f19064 Compare September 25, 2025 03:54
@enjeck

enjeck commented Sep 25, 2025

Copy link
Copy Markdown
Contributor

Here's the problem I was talking about:

Screen.Recording.2025-09-25.at.04.29.15.mov

I introduced the bug anyway, lol. So went ahead and fixed at 2f19064

@enjeck enjeck left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nice!

@enjeck
enjeck merged commit 7ba64b9 into main Sep 25, 2025
61 of 64 checks passed
@enjeck
enjeck deleted the 1769-views-set-mandatory-state-of-a-column branch September 25, 2025 04:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Views: set mandatory-state of a column

3 participants