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: human-readable timestamp of sent newsletters #1590

Merged
merged 4 commits into from
Aug 7, 2024

Conversation

dkoo
Copy link
Contributor

@dkoo dkoo commented Aug 1, 2024

All Submissions:

Changes proposed in this Pull Request:

Fixes a bug where the human-readable timestamp shown next to sent newsletters doesn't respect the site's local timezone.

How to test the changes in this Pull Request:

  1. Ensure your site has a time zone set in Settings > General that's different from GMT/Universal Time.
  2. On trunk, send a newsletter.
  3. Go back to the Newsletters post list and observe that the sent label says something inaccurate. For instance, my site is set to Denver/Mountain Time, and so seconds after sending the newsletter the human-readable timestamp reads like this:
Screenshot 2024-08-01 at 4 20 30 PM
  1. Check out this branch and refresh. Confirm that the timestamp is now more accurate based on when the newsletter was actually sent:
Screenshot 2024-08-01 at 4 23 55 PM

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@dkoo dkoo self-assigned this Aug 1, 2024
@dkoo dkoo requested a review from a team as a code owner August 1, 2024 22:24
Copy link
Member

@miguelpeixe miguelpeixe left a comment

Choose a reason for hiding this comment

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

Works well after the suggested tweak!

includes/class-newspack-newsletters.php Outdated Show resolved Hide resolved
includes/class-newspack-newsletters.php Outdated Show resolved Hide resolved
@dkoo dkoo requested a review from miguelpeixe August 6, 2024 17:23
@github-actions github-actions bot added [Status] Approved Ready to merge and removed [Status] Needs Review labels Aug 6, 2024
@miguelpeixe
Copy link
Member

Not sure why tests started failing now 🤔

@dkoo
Copy link
Contributor Author

dkoo commented Aug 6, 2024

Not sure why tests started failing now 🤔

It's because $sent could have been false if the post didn't have the newsletter_sent meta. 442c522 should fix this by explicitly checking for publish or private post status. Also, even if a post already has the newsletter_sent meta, it could be the wrong date, so we want to overwrite it in that case if it doesn't equal the actual publish date timestamp.

@dkoo dkoo requested a review from miguelpeixe August 6, 2024 20:46
Copy link

codecov bot commented Aug 6, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 17.71%. Comparing base (25ecc21) to head (442c522).
Report is 37 commits behind head on trunk.

Files Patch % Lines
includes/class-newspack-newsletters.php 75.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##              trunk    #1590      +/-   ##
============================================
+ Coverage     17.69%   17.71%   +0.02%     
- Complexity     2327     2330       +3     
============================================
  Files            43       43              
  Lines          8784     8784              
============================================
+ Hits           1554     1556       +2     
+ Misses         7230     7228       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dkoo dkoo merged commit c3ddd29 into trunk Aug 7, 2024
12 checks passed
@dkoo dkoo deleted the fix/newsletter-sent-time-ago branch August 7, 2024 15:51
matticbot pushed a commit that referenced this pull request Aug 15, 2024
# [3.1.0-alpha.1](v3.0.0...v3.1.0-alpha.1) (2024-08-15)

### Bug Fixes

* add preview text fallback to make sure HTML is removed ([b4de251](b4de251))
* human-readable timestamp of sent newsletters ([#1590](#1590)) ([c3ddd29](c3ddd29))
* **subscription-block:** remove default list when misconfigured ([#1608](#1608)) ([6d0b440](6d0b440))

### Features

* consolidate data flows ([#1602](#1602)) ([dd56ab8](dd56ab8)), closes [#1567](#1567) [#1593](#1593) [#1601](#1601)
@matticbot
Copy link
Contributor

🎉 This PR is included in version 3.1.0-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Aug 26, 2024
# [3.1.0](v3.0.1...v3.1.0) (2024-08-26)

### Bug Fixes

* add preview text fallback to make sure HTML is removed ([b4de251](b4de251))
* human-readable timestamp of sent newsletters ([#1590](#1590)) ([c3ddd29](c3ddd29))
* **subscription-block:** remove default list when misconfigured ([#1608](#1608)) ([6d0b440](6d0b440))

### Features

* consolidate data flows ([#1602](#1602)) ([dd56ab8](dd56ab8)), closes [#1567](#1567) [#1593](#1593) [#1601](#1601)
@matticbot
Copy link
Contributor

🎉 This PR is included in version 3.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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.

3 participants