Skip to content

Add an option to tune notifications on account inserts in LdapSyncCommand#1211

Merged
vladsf merged 20 commits intoeventum:masterfrom
vladsf:add-notify-users
Sep 28, 2021
Merged

Add an option to tune notifications on account inserts in LdapSyncCommand#1211
vladsf merged 20 commits intoeventum:masterfrom
vladsf:add-notify-users

Conversation

@vladsf
Copy link
Copy Markdown
Contributor

@vladsf vladsf commented Sep 25, 2021 •
edited
Loading

Add a property to disable user notification on account insert action in LdapAdapter.php
Justification: This adapter already has notification turned off for user account updates. To add the same option for User::insert() is in accordance with the code.

It is debatable to add this option to global ldap config or not.

This PR helps to make notofications configurable in eventum:ldap:sync command.

@vladsf vladsf changed the title WIP: Add a property to disable user notification on account insert WIP: Allow to turn off notifications on account insert in LdapAdapter Sep 25, 2021
@glensc
Copy link
Copy Markdown
Member

glensc commented Sep 25, 2021

how do you tune this flag? via command-line options to sync command?

also, use github draft feature rather title prefix:

@vladsf vladsf changed the title WIP: Allow to turn off notifications on account insert in LdapAdapter Allow to turn off notifications on account insert in LdapAdapter Sep 25, 2021
@vladsf vladsf marked this pull request as draft September 25, 2021 20:19
@vladsf
Copy link
Copy Markdown
Contributor Author

vladsf commented Sep 25, 2021

Yes, I want to add --no-notify option to the ldap sync command.

@vladsf vladsf changed the title Allow to turn off notifications on account insert in LdapAdapter Add an option to tune notifications on account inserts in LdapSyncCommand Sep 27, 2021
@vladsf vladsf marked this pull request as ready for review September 27, 2021 11:25
@vladsf
Copy link
Copy Markdown
Contributor Author

vladsf commented Sep 28, 2021

@glensc I'd like to merge it.

@glensc
Copy link
Copy Markdown
Member

glensc commented Sep 28, 2021

@vladsf will you rebase and fix your PR commit history, or have it squash merge again?

@vladsf
Copy link
Copy Markdown
Contributor Author

vladsf commented Sep 28, 2021 •
edited
Loading

I'd prefer to squash. Single small commit would be better in this case.

@glensc
Copy link
Copy Markdown
Member

glensc commented Sep 28, 2021

@vladsf ambiguity, unclear if you prefer to squash your commits yourself, or squash during merge?

dependabot Bot and others added 13 commits September 28, 2021 16:07
@dependabot @vladsf
Bump laminas/laminas-mail from 2.14.1 to 2.15.1 (eventum#1212)
5f674b7 
Bumps [laminas/laminas-mail](https://github.com/laminas/laminas-mail) from 2.14.1 to 2.15.1.
- [Release notes](https://github.com/laminas/laminas-mail/releases)
- [Commits](laminas/laminas-mail@2.14.1...2.15.1)

---
updated-dependencies:
- dependency-name: laminas/laminas-mail
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@dependabot @vladsf
Bump symfony/flex from 1.15.4 to 1.16.2 (eventum#1213)
66c787e 
Bumps [symfony/flex](https://github.com/symfony/flex) from 1.15.4 to 1.16.2.
- [Release notes](https://github.com/symfony/flex/releases)
- [Commits](symfony/flex@v1.15.4...v1.16.2)

---
updated-dependencies:
- dependency-name: symfony/flex
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@glensc @vladsf
Fix changelog formatting
4a9852b
@glensc @vladsf
Add deprecation for Application class, eventum#1040
6380383
@glensc @vladsf
Use shorter OutputInterface::VERBOSITY constants from BaseCommand
43d1c51
@vladsf
Add example command
fe8d769
@glensc @vladsf
Move command to example extension and autowire it
ad26050
@glensc @vladsf
Update changelog
5bfa329
@glensc @vladsf
Move migrate_storage_adapter comment to class comment
61a3bb2
@glensc @vladsf
Move extension code to src subdir, per Symfony recommended structure
9283223
@glensc @vladsf
Rename Subscriber to EventSubscriber
3856db7
@glensc @vladsf
Move Console/Command to Command
91d0c9b
@vladsf
Add changelog entry
2decad1
@vladsf vladsf merged commit 1ce07a9 into eventum:master Sep 28, 2021
@glensc
Copy link
Copy Markdown
Member

glensc commented Sep 28, 2021 •
edited
Loading

Ok. I'm going to remove @vladsf your merge mana, as obviously you have no idea what are you doing. if you were uncertain, you would have asked, but you instead merged those foreign commits not related to this pull request.

image

@vladsf
Copy link
Copy Markdown
Contributor Author

vladsf commented Sep 28, 2021

OK, I guess I need some contributor guidelines. git diff master showed me only my changes so I pushed and click Merge.

glensc pushed a commit that referenced this pull request Sep 28, 2021
@vladsf @glensc
Add a property to disable user notification on account insert. #1211
ee5e622
@glensc
Copy link
Copy Markdown
Member

glensc commented Sep 28, 2021

because master is your local master, not origin/master

image

@vladsf
Copy link
Copy Markdown
Contributor Author

vladsf commented Sep 28, 2021 •
edited
Loading

Sorry. Github allows me to revert the changes.

@glensc
Copy link
Copy Markdown
Member

glensc commented Sep 28, 2021

Luckily, I disabled your access before you do that "revert changes", which just creates another set of commits with a merge commit.

I've reset master to 386214f before your merge and squashed changes from this PR to one commit ee5e622.

@glensc glensc mentioned this pull request Sep 28, 2021
Closed
@vladsf
Copy link
Copy Markdown
Contributor Author

vladsf commented Sep 28, 2021

Now I have src/Console/Command/ExampleHelloCommand.phpin my pull requests. I guess I need to reset my master to the same..

@vladsf
Copy link
Copy Markdown
Contributor Author

vladsf commented Sep 28, 2021 

$ git reset HEAD^ --hard (x7 times)
$ git push mathnet -f

resolved my issues, I can submit clean PR again.

The question left - should I git rebase upstream/master or git merge upstream/master before starting a new PR?

@vladsf
Copy link
Copy Markdown
Contributor Author

vladsf commented Sep 28, 2021

I am afraid this history rewriting might affect forked repositories.

@glensc
Copy link
Copy Markdown
Member

glensc commented Sep 28, 2021

git rebase, no merge commits to merge requests!

instead of git reset HEAD^ 7 times, do git reset origin/master (or upstream/master) or anything that points to a commit.

@glensc
Copy link
Copy Markdown
Member

glensc commented Sep 28, 2021

history rewriting is done only on feature branches (pull requests). this git reset to undo your damage was one time only, unlikely anyone but you had pulled those changes.

@glensc glensc added this to the 3.10.7 milestone Dec 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Milestone

3.10.7

Development

Successfully merging this pull request may close these issues.

2 participants

@vladsf @glensc