Pro issue 5418 / Use WordPress date and time settings for entry timestamp in edit/view entry sidebar#2118
Conversation
WalkthroughThe changes involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Sidebar
participant FrmAppHelper
User->>Sidebar: Request Entry Timestamps
Sidebar->>FrmAppHelper: get_formatted_time(date, date_format, time_format)
FrmAppHelper->>FrmAppHelper: add_time_to_date(time_format, date)
FrmAppHelper-->>Sidebar: Return Formatted Date and Time
Sidebar-->>User: Display Timestamps
Possibly related PRs
Warning Rate limit exceeded@Crabcyborg has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)tests/cypress/e2e/Entries/EntriesPageDataValidations.cy.js (1)
The regex pattern has been correctly updated to validate the new timestamp format that uses "at" instead of "@" and includes AM/PM indicators, aligning with the PR's objective to use WordPress date and time settings. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
classes/views/frm-entries/_sidebar-shared-pub.php (2)
16-23: Consider enhancing date format handling robustnessWhile the implementation works, consider these improvements:
- Handle both uppercase 'F' and lowercase 'f' month formats
- Validate the date format string to ensure it's valid
- Consider making the month abbreviation configurable rather than hardcoding it
Here's a suggested improvement:
$date_format = get_option( 'date_format' ); if ( $date_format ) { // Use short months since the sidebar space is limited. - $date_format = str_replace( 'F', 'M', $date_format ); + $date_format = str_ireplace( array( 'F', 'f' ), 'M', $date_format ); + // Validate the format string + if ( ! FrmAppHelper::is_valid_date_format( $date_format ) ) { + $date_format = __( 'M j, Y', 'formidable' ); + } } else { // Fallback if there is no option in the database. $date_format = __( 'M j, Y', 'formidable' ); }
43-43: Consider validating the updated_at timestampWhile the code checks for existence and inequality with created_at, it might be good to validate the timestamp format to prevent potential display issues with invalid dates.
- '<b>' . esc_html( FrmAppHelper::get_formatted_time( $entry->updated_at, $date_format ) ) . '</b>' + '<b>' . esc_html( FrmAppHelper::is_valid_timestamp( $entry->updated_at ) ? + FrmAppHelper::get_formatted_time( $entry->updated_at, $date_format ) : + __( 'Invalid date', 'formidable' ) ) . '</b>'classes/helpers/FrmAppHelper.php (1)
2698-2702: LGTM! Consider extracting the " at " string to a constant.The new
add_time_to_datemethod is well-structured and handles time formatting appropriately. For improved maintainability, consider extracting the " at " string to a class constant since it's a UI text that may need to be modified in the future.+ private const TIME_SEPARATOR = ' at '; private static function add_time_to_date( $time_format, $date ) { if ( empty( $time_format ) ) { $time_format = get_option( 'time_format' ); } $trimmed_format = trim( $time_format ); $time = ''; if ( $time_format && ! empty( $trimmed_format ) ) { - $time = ' ' . __( 'at', 'formidable' ) . ' ' . self::get_localized_date( $time_format, $date ); + $time = ' ' . __( 'at', 'formidable' ) . self::TIME_SEPARATOR . self::get_localized_date( $time_format, $date ); } return $time; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
classes/helpers/FrmAppHelper.php(1 hunks)classes/views/frm-entries/_sidebar-shared-pub.php(2 hunks)
🔇 Additional comments (2)
classes/views/frm-entries/_sidebar-shared-pub.php (2)
8-9: LGTM!
The variable fallback is implemented correctly.
29-29: Verify timezone handling in the new time formatting implementation
The switch to get_formatted_time() looks good, but we should ensure it properly handles timezone conversions like the previous implementation.
✅ Verification successful
Let me gather more information about the timezone handling implementation.
Let me check the complete implementation of get_formatted_time to understand how it handles dates.
Let me check the implementation of get_localized_date since it seems to be handling the actual date formatting.
Timezone handling is properly implemented in the new code
The get_formatted_time() implementation correctly handles timezone conversion through WordPress core functions:
- Uses
get_date_from_gmt()to convert from GMT to local timezone - Uses
date_i18n()for localized date formatting - Respects WordPress date and time format settings via
get_option()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for timezone handling in the new method
ast-grep --pattern 'function get_formatted_time($_) {
$$$
}'
# Look for timezone-related code in the helper class
rg -l "timezone|get_option.*timezone|date_default_timezone" --type php
Length of output: 178
Script:
#!/bin/bash
# Check the implementation in FrmAppController.php
rg -A 10 "timezone|get_option.*timezone|date_default_timezone" classes/controllers/FrmAppController.php
# Look for the actual get_formatted_time implementation
rg -A 5 "function get_formatted_time" --type php
Length of output: 980
Script:
#!/bin/bash
# Get the full implementation of get_formatted_time
rg -B 2 -A 20 "function get_formatted_time" classes/helpers/FrmAppHelper.php
# Look for any WordPress date/time related functions being used
rg "get_option\( '(date_format|time_format|timezone|gmt_offset)" classes/helpers/FrmAppHelper.php
Length of output: 969
Script:
#!/bin/bash
# Get the implementation of get_localized_date
rg -B 2 -A 15 "function get_localized_date" classes/helpers/FrmAppHelper.php
# Also check if there are any timezone-related functions being used
rg "date_default_timezone_get|wp_timezone|get_timezone" classes/helpers/FrmAppHelper.php
Length of output: 628
Crabcyborg
changed the title
Crabcyborg
changed the title
lauramekaj1
left a comment
lauramekaj1
left a comment
There was a problem hiding this comment.
@Crabcyborg I tested it and it's working as expected. Thank you!
|
Thank you Laura! 🚀 |
2 participants
Fixes https://github.com/Strategy11/formidable-pro/issues/5418
Now I'm using our more commonly used
get_formatted_timefunction that adds the time data, and some custom logic for the date format (I change long month names to the short month name so if you have it set to show "November", it will show "Nov" instead to save space).This generally uses the WordPress setting, and not our global setting in Pro, as the Pro settings are all formats like
Y/m/dandY-m-d, none of which look as good as the more verbose format options that you can use in the WordPress date setting.The
@is nowatby default, but I think that's okay. It makes it more consistent with our other dates with times which is more important than the possible space saved.Using the following settings These are set at /wp-admin/options-general.php
Before
After
