Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Name validation error while trying to save an edited command #76

Merged
merged 4 commits into from
Feb 2, 2024
Merged

Fix: Name validation error while trying to save an edited command #76

merged 4 commits into from
Feb 2, 2024

Conversation

DieWallSoCom
Copy link

@DieWallSoCom DieWallSoCom commented Sep 21, 2023

Fix: While trying to save an edited command from the command list (command-scheduler/list) an validation error occurs because a command with the same already exists.

The cause of this Problem is the "UniqueEntity" annotation found in the "ScheduledCommand" entity. Its irrelevant whether a new scheduled command is created or an existing one is edited.
This issue is fixed by using a validation group. The "UniqueEntity" constraint now has the validation group "new". This group is added to the form for creating a command, but not to the form to edit an existing command.

To reproduce the fixed problem you have to edit a command in the command list on "/command-scheduler/list". Change something other then the name and try to save the change. It will not work and a note will appear near the name field on the form that will say something like "the name already exists". To work around that issue you have to change the name every time you want to edit a command, even if only to change the cron expression, for example.
The cause of this Problem is the symfony constraint "UniqueEntity". Even if a form is edited (the record shall only be changed) it will see that a command with the same name already exists in the scheduled_command table. The fix will enable the "UniqueEntity" constraint only for a form to create a new command, but not if an existing command is edited.

…slation for the flash message to start a command immediately
@DieWallSoCom DieWallSoCom changed the title Fix: Name validation error while trying to save an edited command details Fix: Name validation error while trying to save an edited command Sep 21, 2023
@Chris53897
Copy link
Collaborator

@DieWallSoCom
Can you please make a seperate PR for the translation? That change is unrelated.

Can you describe a bit more specific how i can reproduce the issue?

@codecov
Copy link

codecov bot commented Sep 25, 2023

Codecov Report

Merging #76 (8a1568a) into master (a9e0854) will increase coverage by 0.12%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master      #76      +/-   ##
============================================
+ Coverage     82.88%   83.01%   +0.12%     
- Complexity      276      277       +1     
============================================
  Files            30       30              
  Lines          1239     1248       +9     
============================================
+ Hits           1027     1036       +9     
  Misses          212      212              
Files Changed Coverage Δ
Entity/ScheduledCommand.php 88.76% <ø> (ø)
Controller/DetailController.php 91.30% <100.00%> (+1.83%) ⬆️
Form/Type/ScheduledCommandType.php 100.00% <100.00%> (ø)

@DieWallSoCom
Copy link
Author

DieWallSoCom commented Sep 25, 2023

I removed the translation change and will make a separate pull request for it.
Additionally I added additional text for the pull request to describe how to reproduce the issue.
I wanted to ask if there will be a version 5.0.8 in the near future.

@Chris53897
Copy link
Collaborator

Thanks. i will have a closer look as soon i have some free time.
Master is planned to be a new major Version 6.
I will backport some changes for the branch 5.x. I guess this PR is the important one for 5.0.8 for you.

@DieWallSoCom
Copy link
Author

Thank you for your time.
It would be great if all my pull requests could be found in a future version like 5.0.8.
I found an inadequacy in my fix. If someone tries to edit a command and changes the name an exception could be thrown if a name of a command that already exists is used. I added a form event to set the name field to "readonly" in the form to edit an existing command. If the form to create a new command is opened the name field can be changed and the validator still works like expected.

@Chris53897
Copy link
Collaborator

I tried to reproduce this. But i do not know how.

The name is unique on purpose. As the name is used as identifier in cli commands.
Every existing command in your database should already has a unique name.
Is that true in your case?

I see no edit button in the list view. To edit a command with a form, you have to call /command-scheduler/detail/edit/ID
Do you mean that page. Or an action like lock/unlock?

If i create or edit a command, the name is a required field. If the name is already taken there is a validation error, that the given value is already used. That is the exact desired behaviour.

Can you please post composer show ?

@DieWallSoCom
Copy link
Author

First, I'm using the version 5.0.7. Maybe I didn't provide this information.

As for the edit button. It's not a usual button. First you have to click on the "plus"-icon in the command list on the leftmost column and then on the "pencil"-icon. The url will be something like "/command-scheduler/detail/edit/ID".

Now I often want to modify the cron expression of an existing command.
But when I change the cron-expression and try to save the edited command I get a message above the name-field that the command name already exists. So I have to change the command name, too.
But that is not what I want. I want to keep the name of the command and only change its cron expression.
Currently I have to edit a command two times to change only the cron expression. The first time I have to change the cron expression and the name of the command. And the second time I change the name back to the original command name.

Wouldn't it be better if a command could be edited one time for changing the cron expression.

I think I will prepare a few screenshots for clarification. I will try to post them this week.

@DieWallSoCom
Copy link
Author

I made a video to clarify the issue I'm having:

Command Scheduler Scheduled commands - Google Chrome 2023-10-13 10-54-51.zip

In the video you can see that the command Example Command 1 is being edited. The only thing that needs to be changed is the cron expression and I would like to keep the command name.
But when I try to save the command after changing its cron expression I get the validation error "This value is already used" for the name field.
To save the changed command I also have to change the command name. In the video I changed it to Example Command 2 and saved it. Next I edited the same command again and changed its name back to "Example Command 1".
Overall I had to edit a command two times only to change its cron expression.

The UniqueConstraint in the ScheduledCommand entity prevents the usage of the same name in the form, even if the form is used to edit a command and not to create a new one.

My PullRequest modifies the form, so that the UniqueConstraint is only used when a new command is created but not when it's edited so that the name of the command does not have to be changed while editing it.

Further I added modifications to disable the name field in the form while editing a command. This was necessary because the UniqueConstraint validator is not used while editing a command. A user could change the name of the command while editing it and choose a name that is already used for another command.
If the form is saved afterwards he would get a response with a status 500 because doctrine would not allow the same name to be used twice in the ScheduledCommand table.
To prevent this error while saving an edited command I disabled the name field in the command edit form.

@Chris53897
Copy link
Collaborator

Chris53897 commented Jan 27, 2024

Maybe the custom entity manager was the missing info, because i could not recreate the problem on my side.
#102

@DieWallSoCom Do you use a custom entity manager?

@Chris53897 Chris53897 merged commit e7e50b8 into Dukecity:main Feb 2, 2024
8 of 9 checks passed
@DieWallSoCom
Copy link
Author

DieWallSoCom commented Feb 2, 2024

@Chris53897
Hi,
sorry for the late answer. I added the entities of the command-scheduler package to the entity manager configuration of my application. Here is part of the doctrine.yaml configuration of my application:

...
        entity_managers:
            default:
                connection: dynamic
                naming_strategy: doctrine.orm.naming_strategy.underscore
           ...
            main:
                connection: main
                naming_strategy: doctrine.orm.naming_strategy.underscore
                mappings:
                    main:
                        is_bundle: false
                        dir: "%kernel.project_dir%/src/Entity/main"
                        prefix: 'App\Entity\main'
                    DukecityCommandSchedulerBundle: ~
...

Does it answer your question? Do you use a different doctrine configuration for the entity manager?

@Chris53897
Copy link
Collaborator

Thanks for sharing, that helps me.

Yes i use it under the default entity manager.
You have a custom entity manager main

This Bundle needs more tests for this
#108

I have integrated it in 5.x and 6.x
But before i will make new releases, i need to do some tests.

@Chris53897
Copy link
Collaborator

I did test 5.x and 6.x with my default entity manager. Looks good.
It did not it with a custom entity manager. But i made new releases.

@DieWallSoCom @fautor
Can you please check it it works correct?

@DieWallSoCom
Copy link
Author

Hi,

it works for me. Now I can edit commands without the need to rename them.

@Chris53897: Would it be possible to add the following prs to the 5.x-branch?
#77
#78
They were added in the main-branch as far as I could see, but not in the 5.x-branch. They contains fixes for German translations.

@Chris53897
Copy link
Collaborator

Can you please provide a PR against 5.x Branch?
I will merge it quickly.

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.

2 participants