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 #13920 changed behavior send from hour introduced in 5.1 #14089

Open
wants to merge 2 commits into
base: 5.1
Choose a base branch
from

Conversation

magician7
Copy link

@magician7 magician7 commented Sep 2, 2024

Q A
Bug fix? (5.1) 🟢
New feature/enhancement? (use the a.x branch) 🔴
Deprecations? 🔴
BC breaks? (it brings back behavior from 5.0.4) 🔴🟢
Automated tests included? 🔴
Issue(s) addressed Fixes #13920

Description

Changes introduced in 5.1 (magician7@894929c) brakes the functionality of executing campaigns when set "Start from hour".
This push request brings this back to previous behavior. When you set Start from specific hour was in 5.1:
if it is earlier then it's planned for a specific hour and fired up correctly BUT
when it is later then it's scheduled for the selected hour BUT for NEXT day.

Now it works as before.

When it is earlier than it's planned for a specific hour and fired up correctly AND
when it is later, then it fires immediately.

Changes introduced in 5.1 (894929c) brakes the functionality of executing campaigns when set "Start from hour".
This push request brings this back to previous behavior. When you set Start from specific hour was in 5.1:
if it is earlier then it's planned for a specific hour and fired up correctly BUT
when it is later then it's scheduled for the selected hour BUT for NEXT day.

Now it works as before.

When it is earlier than it's planned for a specific hour and fired up correctly AND
when it is later, then it fires immediately.
@escopecz
Copy link
Member

escopecz commented Sep 2, 2024

I'm afraid this PR contains more changes than expected. Can you please update the PR so it contains on the the change you are proposing?

@escopecz escopecz added the WIP PR's that are not ready for review and are currently in progress label Sep 2, 2024
@magician7
Copy link
Author

magician7 commented Sep 2, 2024

I'm afraid this PR contains more changes than expected. Can you please update the PR so it contains on the the change you are proposing?

Yes, now I can see that. This is my first pull request and I definitly made a mistake. My PR should modify only one file. Commit 51c71e2.

I'll try to correct this.

I'm open if you have suggestion how to do this. Next time will be better then ;-)

EDIT: Ok, now It's ok I suppose.

@magician7 magician7 changed the base branch from 5.x to 5.1 September 2, 2024 15:30
@escopecz
Copy link
Member

escopecz commented Sep 2, 2024

I'm not sure what you did. But the best way to create a PR is to:

  1. git checkout 5.x to get to the branch with the latest changes
  2. git pull to make sure you have the latest changes
  3. make the changes, commit, push

Now that you are in this state, I'd recommend to:

  1. Make a backup of your branch just in case something goes wrong with git checkout fix_13920-changed-behavior-send-from-hour and git checkout fix_13920-changed-behavior-send-from-hour-backup
  2. Go back to this branch with git checkout fix_13920-changed-behavior-send-from-hour
  3. Reset to the latest 5.x branch with git fetch upstream I assume the mautic/mautic repository is called upstream on your local. Confirm that with git remote -v and update the name of the remote accordingly. Then perform the hard reset with git reset --hard upstream/5.x again, update the remote name if necessary.
  4. Cherry-pick your commit with git cherry-pick 51c71e2917cf25e8070ea01b11bc75cde1c82ee3
  5. Force-push with git push --force

But in order to accept these changes it will have to be covered by PHPUNIT tests. Ideally a functional test.

@magician7
Copy link
Author

magician7 commented Sep 2, 2024

Thank you for your specific instructions.

In this commit, I only removed unnecessary code from one file which caused this problem.

@RCheesley RCheesley marked this pull request as draft September 2, 2024 17:29
@magician7 magician7 marked this pull request as ready for review September 2, 2024 21:46
@escopecz
Copy link
Member

escopecz commented Sep 3, 2024

@rahuld-dev can you please look at this change as you are the original author (#13263)? Why was this code added?

@escopecz escopecz added bug Issues or PR's relating to bugs campaigns Anything related to campaigns and campaign builder and removed WIP PR's that are not ready for review and are currently in progress labels Sep 3, 2024
@magician7
Copy link
Author

What if @rahuld-dev doesn`t response? Any chance to approve this PR?

@rahuld-dev
Copy link
Contributor

@magician7 Will check and update here...Thanks!

@magician7
Copy link
Author

Ehm.., ehm... little reminder here ;-) @rahuld-dev

@kuzmany
Copy link
Member

kuzmany commented Oct 24, 2024

@rahuld-dev can we move with this task please?

@magician7
Copy link
Author

Any progres?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues or PR's relating to bugs campaigns Anything related to campaigns and campaign builder
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segment change schedule works differently in 5.1 than 5.0.4 - different aproach to "Send from" hour.
4 participants