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 #9875 SugarFeed shows 0 seconds ago and negative interval for cer… #9876

Merged
merged 1 commit into from Apr 14, 2023

Conversation

abuzarfaris
Copy link
Contributor

@abuzarfaris abuzarfaris commented Dec 27, 2022

…tain datetime formats

Use proper user datetime format to convert datetime string to timestamp in SugarFeed.php

Description

use date_create_from_format with current user datetime format instead of strtotime because strtotime to time does lots of assumtions e.g if the separator is a slash (/), then the American m/d/y is assumed. If the separator is a dash (-) or a dot (.), then the European d-m-y format is assumed

Motivation and Context

SugarFeeds shows correctly for all users with different datetime format settings

How To Test This

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Final checklist

  • My code follows the code style of this project found here.
  • My change requires a change to the documentation.
  • I have read the How to Contribute guidelines.

@SuiteBot
Copy link

This pull request has been mentioned on SuiteCRM. There might be relevant details there:

https://community.suitecrm.com/t/bug-in-my-activity-stream/87625/2

@pgorod
Copy link
Contributor

pgorod commented Jan 14, 2023

Thanks!
May I suggest some error handling on the return of date_create_from_format?

It's an alias of DateTime::createFromFormat

Return Values

Returns a new DateTime instance or false on failure.

We should probably check if the function returned successfully...

@abuzarfaris
Copy link
Contributor Author

Thanks! May I suggest some error handling on the return of date_create_from_format?

It's an alias of DateTime::createFromFormat

Return Values

Returns a new DateTime instance or false on failure.

We should probably check if the function returned successfully...
I have added proper logging incase startdate is not parsed

@pgorod
Copy link
Contributor

pgorod commented Feb 2, 2023

Thanks @abuzarfaris !

@serhiisamko091184
Copy link
Contributor

Hello @abuzarfaris
thanks for your contribution,

would you be so kind to squash commits in your branch into one (it means to combine multiple commits into single commit)?
Thanks a lot in advance.

Regards,
Serhii

@serhiisamko091184 serhiisamko091184 added Status: Requires Code Review Needs the core team to code review Status:Assessed PRs that have been tested and confirmed to resolve an issue by a core team member Branch:Hotfix labels Feb 8, 2023
@abuzarfaris
Copy link
Contributor Author

Hello @abuzarfaris thanks for your contribution,

would you be so kind to squash commits in your branch into one (it means to combine multiple commits into single commit)? Thanks a lot in advance.

Regards, Serhii

I have tried to squash can you check if it's correct

@clemente-raposo clemente-raposo added the Status:Requires Updates Issues & PRs which requires input or update from the author label Feb 15, 2023
@clemente-raposo
Copy link
Contributor

clemente-raposo commented Feb 15, 2023

Hi @abuzarfaris, thank you once again.

There shouldn't be any Merge commits like the following. Because we use rebasing instead of merging (which gives a way cleaner and easier to manage history timeline)

image

@abuzarfaris abuzarfaris force-pushed the bugfix_9875 branch 2 times, most recently from 556450f to 09a542c Compare February 15, 2023 15:53
@abuzarfaris
Copy link
Contributor Author

abuzarfaris commented Feb 15, 2023

Hi @abuzarfaris, thank you once again.

There shouldn't be any Merge commits like the following. Because we use rebasing instead of merging (which gives a way cleaner and easier to manage history timeline)

image

@clemente-raposo Fixed the merge issue and squashed the commits

@clemente-raposo clemente-raposo added the Status:In Review Pull Requests that are activity being reviewed by the core team label Mar 3, 2023
…rval for certain datetime formats

Use proper user datetime format to convert datetime string to timestamp in SugarFeed.php
@clemente-raposo clemente-raposo added Status: Requires Testing Requires Manual Testing Status: Passed Code Review Mark issue has passed code review reviewed and removed Status: Requires Code Review Needs the core team to code review Status:Requires Updates Issues & PRs which requires input or update from the author Status:In Review Pull Requests that are activity being reviewed by the core team labels Mar 3, 2023
Copy link
Contributor

@johnM2401 johnM2401 left a comment

Choose a reason for hiding this comment

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

LGTM, Checked each Time and Date Format, appears to as though Hours/Minutes renders as expected.

@johnM2401 johnM2401 added Status: Passed Testing and removed Status: Requires Testing Requires Manual Testing labels Apr 11, 2023
@jack7anderson7 jack7anderson7 merged commit f345fe1 into salesagility:hotfix Apr 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Branch:Hotfix Status:Assessed PRs that have been tested and confirmed to resolve an issue by a core team member Status: Passed Code Review Mark issue has passed code review reviewed Status: Passed Testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants