Skip to content

Conversation

@artlog
Copy link

@artlog artlog commented Aug 3, 2025

Variant Generic adapted/merged to main
Following #4199

Variant generic is another choice than Poll type where chosen_rank is a json array of possible configurable text answers from poll creator/owner.

creating

Capture d’écran du 2025-08-02 23-47-44

selecting possible answers at creation time in configuration tab :

image

then polls are selectable

Currently used to be exported and computed outside nextcloud poll with csv, results are not visible nor computed in poll.

VoteVariant type in front was renamed VotingVariant to match php code naming. Choice to use VoteVariant into php could have been done too, prefering to keep api side naming. Unification seems needed for Api poll calls.

Hoping this is clearer now.

@artlog artlog marked this pull request as ready for review August 3, 2025 16:10
@artlog artlog force-pushed the variant_generic_unified_main branch from 177a77b to fc01f20 Compare August 3, 2025 16:15
@dartcafe
Copy link
Collaborator

dartcafe commented Aug 3, 2025

VoteVariant type in front was renamed VotingVariant to match php code naming. Choice to use VoteVariant into php could have been done too, prefering to keep api side naming. Unification seems needed for Api poll calls.

Yes. Should be consistent. Changing the backend naming would affect the database, too. So this decision was the better one.

Hoping this is clearer now.

Yep! 👍

@dartcafe
Copy link
Collaborator

dartcafe commented Aug 3, 2025

@artlog. I see you approach the final solution in steps. I like that.

So when ever you think a package is ready, we can merge step by step and give the feature a toggle, so the functionality can be constantly tested without releasing unfinished features.

What do you think?

@artlog
Copy link
Author

artlog commented Aug 3, 2025

this is the idea.
step by step...

@artlog
Copy link
Author

artlog commented Aug 9, 2025

To have a coherent functionality quickly i study this addition

Voting variant generic is disabled by default and should be activated globally by administrator. So there is no visible change at upgrade without an explicit action to enable variant.

@dartcafe
Copy link
Collaborator

@artlog The merge conflict can be solved easily: Just accept both changes.

Not that we are waiting for each other: If you want to merge, request a review by me as a sign, that you are finished with this PR from your perspective.

@dartcafe
Copy link
Collaborator

dartcafe commented Aug 14, 2025

CI looks good so far

Some to dos:

  • Add the new property to the test factory (tests/Unit/Factories/PollFactory.php)
  • run composer run cs:fix
  • Do a rebase and resolve the merge conflict (accept both changes)

@artlog artlog force-pushed the variant_generic_unified_main branch from 9105236 to 4a1b004 Compare August 15, 2025 19:28
@artlog
Copy link
Author

artlog commented Aug 15, 2025

i just aligned existing code with current main and test request, no new change.

Current code add variant generic for which usage is for collecting votes only.
Results should be exported and computed outside nextcloud poll with csv, results are not visible nor computed in poll.

Majority judgement new method won't be done within this merge, but this will be required to provide it,

This does not include creation of new variant disabled by default from my prior comment, i will notify when done.

@dartcafe
Copy link
Collaborator

@artlog If you make a rebase: TableSchema has moved to a version sub dir. I fear you have to solve this conflict manually. As far as I can see, the only change is the addition of your new field to polls.

@artlog artlog force-pushed the variant_generic_unified_main branch from 4a1b004 to 1133211 Compare August 17, 2025 12:46
@artlog
Copy link
Author

artlog commented Aug 17, 2025

@dartcafe this is working on my system, it look like TableSchema change was already merged, did it ?

I had to reset my environment since i got a Not null violation on group_id oc_polls_share , this is working a clean env on 31.0.6 nextcloud version.

  • rebased on main
  • added variants selection disabled by default require to be enable in user settings of poll.
Capture d’écran du 2025-08-17 14-34-57

This was my final change for this, hoping you get this validated soon ;-)

@dartcafe
Copy link
Collaborator

dartcafe commented Aug 17, 2025

Some things left:

  • sign your commits (DCO fails, click on the entry and see how to do it)
  • format/prettier fix (Lint prettier fails)
  • add the new polls property to the test factory (PHPUnit fails)
    Nonsense: It is set. Maybe it is the default = 1 in the TableSchema for the text type, what is confusing the test
  • Add missing copyright info (REUSE Check fails)
  • request a review from me, when finished

@dartcafe
Copy link
Collaborator

Not null violation on group_id oc_polls_share

This was probably #4213

@dartcafe dartcafe self-requested a review August 17, 2025 18:01
@artlog artlog force-pushed the variant_generic_unified_main branch from 1133211 to e09772a Compare August 17, 2025 19:25
@artlog
Copy link
Author

artlog commented Aug 17, 2025

@dartcafe hopefully fixed comments requests

@dartcafe
Copy link
Collaborator

Let's see, what CI says. 😉

@artlog
Copy link
Author

artlog commented Aug 17, 2025

it is a little disapointing... what that's default problem.
looks like all other Types:TEXT are nullable... ( 'notnull' => false, 'default' => null ) while this one is not. will see tomorrow.

@dartcafe
Copy link
Collaborator

it is a little disapointing... what that's default problem.

I am wondering, too. I'll have a look at it. I don't know, what I don't see.

@dartcafe
Copy link
Collaborator

dartcafe commented Aug 17, 2025

looks like all other Types:TEXT are nullable... ( 'notnull' => false, 'default' => null ) while this one is not. will see tomorrow.

Umpf. 🙈 Yes, I think this is the problem, because a text field with default '' is treated as null. If I remember right, it is because of Oracle DB, which has this limitation.

This means chosen_rank has to be nullable.

Quick search: https://www.sqlservercentral.com/forums/topic/oracle-empty-string-is-the-same-as-null
But a lot of other articles can be found

Usually it should be reported by the migrationManager
https://github.com/nextcloud/server/blob/23573c4947a2d9a5db9907a76eb4151985ee6542/lib/private/DB/MigrationService.php#L571-L575

Same problem applies to boolean fields. Obviously Oracle says null = false.

@github-actions
Copy link

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

@artlog
Copy link
Author

artlog commented Aug 18, 2025

i kept '' default value but set it as nullable, hoping this is the good choice.

@dartcafe dartcafe self-requested a review August 18, 2025 10:39
@artlog
Copy link
Author

artlog commented Aug 19, 2025

I patched the poll and added some generic options. This eliminates the error for the poll with generic options, but it stays for existing polls with the usual answers.

how do we proceed ? Can you provide your changes to start from that ? i will install a mariadDB, hoping it really acts the same as a MySql.

@dartcafe
Copy link
Collaborator

dartcafe commented Aug 19, 2025

I patched the poll and added some generic options. This eliminates the error for the poll with generic options, but it stays for existing polls with the usual answers.

I was referring to the runtime error, when chosenRank was an empty string. I just added some voting options to the field in the db.

The CI error is probably caused here

@artlog
Copy link
Author

artlog commented Aug 19, 2025

I patched the poll and added some generic options. This eliminates the error for the poll with generic options, but it stays for existing polls with the usual answers.

I was referring to the runtime error, when chosenRank was an empty string. I just added some voting options to the field in the db.

The CI error is probably caused here

i will try in following days to support correctly nullable since it is actualy the case, chosenRank in standard polls is just empty anyway. and see for array, since it is really an array when used.

philippe lhardy added 15 commits August 20, 2025 09:48
reworked from
[email protected]:vinimoz/polls.git rank-vote-feature-vue3
TRAPPE Vincent <[email protected]>

voting variant generic

a configuration list of answers for a vote.

merged lib/Db/Pool.php with main manually

Signed-off-by: philippe lhardy <[email protected]>
Move code VoteItem code into VoteButton
Add drop-down list in VoteButton
adapt with new poll types

Signed-off-by: philippe lhardy <[email protected]>
Signed-off-by: philippe lhardy <[email protected]>
remove unused code in VoteIndicator
reset VoteIndicator handles only simple variant
move generic variant display in VoteItem

Signed-off-by: philippe lhardy <[email protected]>
address remarks from nextcloud#4199

Signed-off-by: philippe lhardy <[email protected]>
npm run format:fix
make cs-fix

Signed-off-by: philippe lhardy <[email protected]>
add a lost comment during merge

Signed-off-by: philippe lhardy <[email protected]>
To be able to select variant in poll creation option should be checked
within user settings
allow to have this experimental functionality without impacting classical use

Signed-off-by: philippe lhardy <[email protected]>
npm run format:fix

Signed-off-by: philippe lhardy <[email protected]>
- add default string value for TEXT
- add missing license header

Signed-off-by: philippe lhardy <[email protected]>
- don't parse chosenRank for non generic variant
- delegate to existing getChosenRank of getters
- set default table column chosen_rank to null
- set default chosenRank to json empty array

Signed-off-by: philippe lhardy <[email protected]>
@artlog artlog force-pushed the variant_generic_unified_main branch from b58292b to 4dc2cbe Compare August 20, 2025 07:49
@artlog
Copy link
Author

artlog commented Aug 20, 2025

@dartcafe let's give a try like it is now ?

@artlog artlog force-pushed the variant_generic_unified_main branch from e4bfe78 to 0ec37e8 Compare August 20, 2025 08:48
Signed-off-by: philippe lhardy <[email protected]>
@artlog artlog requested a review from dartcafe August 20, 2025 08:50
@artlog
Copy link
Author

artlog commented Aug 22, 2025

@dartcafe : after refereshing my environment with last merge and running occ upgrade i get this error :

Error: Typed property OCA\Polls\Db\V2\DbManager::$schema must not be accessed before initialization in /var/www/html/custom_apps/polls/lib/Db/V2/TableManager.php:466
Stack trace:
#0 /var/www/html/custom_apps/polls/lib/Migration/Version080301Date20250822153002.php(84): OCA\Polls\Db\V2\TableManager->fixNullishShares()
#1 /var/www/html/lib/private/DB/MigrationService.php(499): OCA\Polls\Migration\Version080301Date20250822153002->preSchemaChange(Object(OC\Migration\SimpleOutput), Object(Closure), Array)
#2 /var/www/html/lib/private/DB/MigrationService.php(395): OC\DB\MigrationService->executeStep('080301Date20250...', false)
#3 /var/www/html/lib/private/legacy/OC_App.php(694): OC\DB\MigrationService->migrate()
#4 /var/www/html/lib/private/Updater.php(325): OC_App::updateApp('polls')
#5 /var/www/html/lib/private/Updater.php(236): OC\Updater->doAppUpgrade()
#6 /var/www/html/lib/private/Updater.php(100): OC\Updater->doUpgrade('31.0.6.2', '31.0.6.2')
#7 /var/www/html/core/Command/Upgrade.php(192): OC\Updater->upgrade()
#8 /var/www/html/3rdparty/symfony/console/Command/Command.php(326): OC\Core\Command\Upgrade->execute(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#9 /var/www/html/3rdparty/symfony/console/Application.php(1078): Symfony\Component\Console\Command\Command->run(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#10 /var/www/html/3rdparty/symfony/console/Application.php(324): Symfony\Component\Console\Application->doRunCommand(Object(OC\Core\Command\Upgrade), Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#11 /var/www/html/3rdparty/symfony/console/Application.php(175): Symfony\Component\Console\Application->doRun(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#12 /var/www/html/lib/private/Console/Application.php(187): Symfony\Component\Console\Application->run(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#13 /var/www/html/console.php(87): OC\Console\Application->run(Object(Symfony\Component\Console\Input\ArgvInput))
#14 /var/www/html/occ(33): require_once('/var/www/html/c...')

this is related to #4225

@dartcafe
Copy link
Collaborator

Yep. I accessed thew table schema to check for the existence of a column, but forgot to initialize it.

@artlog
Copy link
Author

artlog commented Sep 28, 2025

@dartcafe what is status of this, will it be integrated or what is expected from me ?
BTW i am at berlin community nextcloud conference until 2th october 2025, if you are around.. i will get some community help here too.

@dartcafe
Copy link
Collaborator

dartcafe commented Oct 3, 2025

I had no change to look into it, since the problem around the migration got priority and my time is limited atm due to some personal tasks and work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants