-
Notifications
You must be signed in to change notification settings - Fork 35
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
Be able to move/add in bulk clients from one group client to another #72
Be able to move/add in bulk clients from one group client to another #72
Conversation
ping @SebSept @akiletour @jf-viguier Ready for review :) |
Thanks for this PR. |
Good job 👍🏼 |
Almost done !
Phpstan will be integrated as a github action and available as a local tool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for info, do you plan to fix this phpstan warnings ?
Hi @SebSept |
Hi @SebSept |
@@ -35,6 +35,11 @@ | |||
*/ | |||
final class CustomersGroups extends Command | |||
{ | |||
public const SUCCESS = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I though this constants were available with this symfony version. But they are not.
It's only for later versions.
It's nice that you added it !
(We'll move them to the base command later.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes !
I noticed that in our version they don't exist, that's why I added them in my class. But I think for the sake of reusability we should put them in the Command class.
phpstan & php-cs-fixer are happy, Nice :) Thanks ! |
Also I think that the word "move" is not appropriate. |
@PululuK Can you just add a more explicit message ? (I wish I could add the message but I'm not really sure of the error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me, the non explicit error message is confusing.
The user deserve to know what was wrong.
I suppose it is related to the fact that there is a need to have users in different groups as mentioned in the PR explanation. But I'm not sure.
Can someone confirm this and write a correct error message ?
I also mention the fact that the command result can be something different than a 'move', user can be added to a group without being removed from the source one.
Because of the current lack of time of reviewer I suggest to merge with this wrong definition.
Unless someone fix it.
Just a last effort to seal your work @PululuK ? 🙏🏼 |
taken from PR description : "have at least two groups and at least one customer associated with one of the groups. Signed-off-by: SebSept <sebastienmonterisi@yahoo.fr>
If we follow naming convention, the command should e named fop:customer:transfert ? or splitted in fop:customer:move and fop:customer:copy ? |
the "group" concept is important ... |
Hello I'm back ! |
Personne pour finaliser ça ? |
fop:customer:group-change it's ok for me |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just update name of Command and it's ok
Finally I think my proposal (
If anyone has something against that choice, it's time to speak, now or never 😆 |
thanks @PululuK (and sorry for the delay). |
Feature request
Be able to move/add in bulk clients from one group client to another
Specs
To be able to transfer customers from a
Group_A
group to aGroup_B
Select groups
1 - be able to select
Group_A
(group from) : only groups with at least one customer2 - be able to select
Group_B
(group to) : The previously selected group must not be in this list.Select action before
Select an action once the clients of group
Group_A
have been transferred to groupGroup_B
1 - Delete
Group_A
group : if we choose this option groupGroup_A
will be deleted after transfer2 - Remove customers in
Group_A
group : if we choose this option customers will be removed from theGroup_A
3 - Just add to new group : if we choose this option users of group
Group_A
will be added to groupGroup_B
4 - Cancel current action : Command exit :)
POC
🎥 Click to see screen record