Skip to content

feat: add saml:user:add command to pre-provision users #962

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

guerby
Copy link

@guerby guerby commented May 20, 2025

feat: add saml:user:add command to pre-provision users, fix #939

@guerby guerby requested a review from blizzz as a code owner May 20, 2025 12:34
@blizzz blizzz changed the title fix 939 feat: add saml:user:add command to pre-provision users May 23, 2025
Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

Some suggestions on user facing text mostly. Please update the commit message to be meaningful, e.g. how i edited the PRs title.

@guerby guerby force-pushed the patch-user-add-fix-939 branch 2 times, most recently from fd0c258 to a68adae Compare May 23, 2025 12:02
Copy link

github-actions bot commented Jun 4, 2025

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

@blizzz
Copy link
Member

blizzz commented Jun 11, 2025

FWIW Github does not create notifications when a commit was added.

@guerby
Copy link
Author

guerby commented Jun 16, 2025

FWIW Github does not create notifications when a commit was added.

I have commited the change to make email VALUE_OPTIONAL, I think this is the last pending review item, let me know if I missed something, thanks!

@blizzz
Copy link
Member

blizzz commented Jun 19, 2025

Thanks! Please update with a rebase, not merging master hereinto.

@guerby guerby force-pushed the patch-user-add-fix-939 branch from 030d421 to 55a16f6 Compare June 19, 2025 11:05
@guerby
Copy link
Author

guerby commented Jun 19, 2025

Thanks! Please update with a rebase, not merging master hereinto.

I have followed the DCO suggestions:

git pull
git rebase HEAD~9 --signoff
git push --force-with-lease origin patch-user-add-fix-939

I hope this is correct :)

@guerby guerby force-pushed the patch-user-add-fix-939 branch 2 times, most recently from 1265555 to 61f344c Compare June 19, 2025 11:12
@blizzz
Copy link
Member

blizzz commented Jun 20, 2025

I use git rebase master instead of git rebase HEAD~9 but it looks good anyhow ✔️

@blizzz
Copy link
Member

blizzz commented Jun 20, 2025

composer i && composer run cs:fix should help solve the last failing test.

Plusses, if the UPDATE $filename commits can be squashed to their parent :)

@guerby
Copy link
Author

guerby commented Jun 20, 2025

composer i && composer run cs:fix

On my debian 12 I did apt-get install composer php-xml php-curl and ran composer i && composer run cs:fix.

For the squash part I'm reading git documentation, I assume you mean commits 29ba794 2cced28 and b886e05 ?

@blizzz
Copy link
Member

blizzz commented Jun 23, 2025

composer i && composer run cs:fix

On my debian 12 I did apt-get install composer php-xml php-curl and ran composer i && composer run cs:fix.

Thanks!

For the squash part I'm reading git documentation, I assume you mean commits 29ba794 2cced28 and b886e05 ?

Aye 🙂

@guerby guerby force-pushed the patch-user-add-fix-939 branch from 316b67f to a93f4f8 Compare June 24, 2025 05:30
@guerby
Copy link
Author

guerby commented Jun 24, 2025

composer i && composer run cs:fix

On my debian 12 I did apt-get install composer php-xml php-curl and ran composer i && composer run cs:fix.

Thanks!

For the squash part I'm reading git documentation, I assume you mean commits 29ba794 2cced28 and b886e05 ?

Aye 🙂

Hi, I think I've done the squash and fix whitespace that composer missed. The other lint errors are not in my code AFAIK.

Let me know if it look ok to you and thanks again!

@blizzz
Copy link
Member

blizzz commented Jun 24, 2025

i think something went wrong, the changes in the l10n should not be here though (they will be imported automatically from the translation tool)

@guerby
Copy link
Author

guerby commented Jun 24, 2025

i think something went wrong, the changes in the l10n should not be here though (they will be imported automatically from the translation tool)

Do you have a suggestion on what to do to fix this PR (remove unwanted changes dragged from probably one of my git commands)?

@guerby guerby force-pushed the patch-user-add-fix-939 branch from a93f4f8 to d1d9dbb Compare June 24, 2025 16:59
@guerby
Copy link
Author

guerby commented Jun 24, 2025

i think something went wrong, the changes in the l10n should not be here though (they will be imported automatically from the translation tool)

I did the following to get rid of the unwanted commits:

git rebase -i HEAD~20 # removed the "pick" lines of all unwanted commits
git push origin patch-user-add-fix-939 --force-with-lease

They seem to have disappeared now.

@guerby guerby force-pushed the patch-user-add-fix-939 branch 2 times, most recently from df159b1 to b2f1c0e Compare June 24, 2025 17:01
@guerby
Copy link
Author

guerby commented Jun 24, 2025

Hmm they seem to reappear immediately, may be some bot is doing something?

@guerby guerby force-pushed the patch-user-add-fix-939 branch from b2f1c0e to 53f767a Compare June 24, 2025 17:07
@blizzz
Copy link
Member

blizzz commented Jun 25, 2025

Hmm they seem to reappear immediately, may be some bot is doing something?

Is your master checkout up to date?

protected function configure(): void {
$this
->setName('saml:user:add')
->setDescription('adds an saml account')
Copy link

Choose a reason for hiding this comment

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

Suggested change
->setDescription('adds an saml account')
->setDescription('Add a SAML account')

guerby and others added 8 commits July 18, 2025 17:20
Imrove user message

Co-authored-by: Arthur Schiwon <[email protected]>
Signed-off-by: Laurent GUERBY <[email protected]>
Signed-off-by: Laurent GUERBY <[email protected]>
Signed-off-by: Laurent GUERBY <[email protected]>
Signed-off-by: Laurent GUERBY <[email protected]>
Signed-off-by: Laurent GUERBY <[email protected]>
@guerby guerby force-pushed the patch-user-add-fix-939 branch from 53f767a to 4866e72 Compare July 18, 2025 15:21
@guerby
Copy link
Author

guerby commented Jul 18, 2025

@blizzz do you have a suggestion on how to make progress?

I did again the following:

git pull origin master --rebase
git push origin patch-user-add-fix-939 --force-with-lease

@guerby
Copy link
Author

guerby commented Jul 18, 2025

Then I cliked on github web "merge" button

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.

Add saml:user:add command to pre-provision users
3 participants