Feature/daniele/enrich debug page #748

DanieleAlessandra · 2024-10-11T08:48:34Z

Enriched Debug Page:

Added: unfiltered_site_url;
Added: Scheduled Crons
Added: Collapsile Tables
Fixed: CSV Logs Download

fajardoleo · 2024-11-28T10:04:38Z

templates/debug.php

-<h2><?php fs_esc_html_echo_inline( 'Actions', 'actions' ) ?></h2>
+<h2>
+    <?php fs_esc_html_echo_inline( 'Actions', 'actions' ) ?>
+</h2>


@DanieleAlessandra I suggest reverting this as the text remains the same.

fajardoleo · 2024-11-28T10:27:20Z

templates/debug.php

-    <h2><?php fs_esc_html_echo_inline( 'Debug Log', 'debug-log' ) ?></h2>
+    <h2>
+        <?php fs_esc_html_echo_inline( 'Debug Log', 'debug-log' ) ?>
+    </h2>


@DanieleAlessandra Similar to this:
https://github.com/Freemius/wordpress-sdk/pull/748/files#r1861868503

fajardoleo · 2024-11-28T10:32:10Z

templates/debug/scheduled-crons.php

-<h1><?php fs_esc_html_echo_inline( 'Scheduled Crons' ) ?></h1>
+<?php echo $title_tag_open; ?>
+<?php
+	if ($display_toggle_button) {


@DanieleAlessandra There are missing spaces here.

fajardoleo · 2024-11-28T10:38:02Z

templates/debug/scheduled-crons.php

-<h1><?php fs_esc_html_echo_inline( 'Scheduled Crons' ) ?></h1>
+<?php echo $title_tag_open; ?>
+<?php
+	if ($display_toggle_button) {


@DanieleAlessandra How about this instead?

<?php if ( $display_toggle_button ) : ?> <button class="toggle-table" aria-expanded="<?php echo $is_open ?>"> <span class="caret-icon">▼</span> </button> <?php endif ?>

Also, in our convention, we don't add a semicolon for single-line PHP code.

fajardoleo · 2024-11-28T10:39:56Z

templates/debug/scheduled-crons.php

@@ -13,6 +13,17 @@
 	$fs_options      = FS_Options::instance( WP_FS__ACCOUNTS_OPTION_NAME, true );
 	$scheduled_crons = array();

+	$title_tag             = isset($VARS['title_tag']) ? $VARS['title_tag'] : 'h1';


@DanieleAlessandra Missing spaces. I recommend reviewing this and adding the relevant spacing accordingly:
https://developer.wordpress.org/coding-standards/wordpress-coding-standards/php/#whitespace

fajardoleo · 2024-11-28T10:41:41Z

templates/debug/scheduled-crons.php

+	if (isset($VARS['is_open'])) {
+		$is_open = $VARS['is_open'] ? 'true' : 'false';
+	}
+	else {


@DanieleAlessandra I recommend reviewing this for control structures and brace styles:
https://developer.wordpress.org/coding-standards/wordpress-coding-standards/php/#brace-style

templates/debug.php

fajardoleo · 2024-11-28T10:48:34Z

templates/debug.php

@@ -888,3 +934,32 @@ class="dashicons dashicons-download"></i> <?php fs_esc_html_echo_inline( 'Downlo
        });
    </script>
 <?php endif ?>
+<script type="text/javascript">
+    // JavaScript to toggle the visibility of the table body and change the caret icon
+    document.addEventListener('DOMContentLoaded', function () {


@DanieleAlessandra Similar to PHP, please review this and improve the spacing accordingly:
https://developer.wordpress.org/coding-standards/wordpress-coding-standards/javascript/#examples-of-good-spacing

fajardoleo · 2024-11-28T11:25:47Z

templates/debug.php

+<script type="text/javascript">
+    // JavaScript to toggle the visibility of the table body and change the caret icon
+    document.addEventListener('DOMContentLoaded', function () {
+        document.querySelectorAll('.toggle-table').forEach(function (button) {


@DanieleAlessandra The JS code just above this block uses jQuery. Is there any reason why we can't do the same here?

While the codebase still contains some jQuery usage, I believe it is beneficial for us to reduce dependency on it over time, as it’s likely to be phased out in the future.

For this specific block, I opted not to use jQuery because it is entirely new and independent from the other scripts on the page. Additionally, the functionality is simple enough to implement without relying on the library.

fajardoleo · 2024-11-28T14:21:34Z

includes/class-fs-logger.php

+		 * @return bool
+		 */
+		private static function write_csv_to_filesystem( $file_path, $query_results ) {
+			require_once( ABSPATH . 'wp-admin/includes/file.php' );


@DanieleAlessandra I think it should be like this (please double-check):

private static function write_csv_to_filesystem( $file_path, $query_results ) { if ( empty( $query_results ) ) { return false; } WP_Filesystem(); global $wp_filesystem; if ( ! $wp_filesystem->is_writable( dirname( $file_path ) ) ) { return false; } $headers = array_keys( (array) $query_results[0] );

This way, we won't trigger the other lines if we will return false anyway.

Also, the require_once line doesn't appear to be needed, as we are in a WordPress admin context where that is already included.

fajardoleo · 2024-11-28T14:35:30Z

includes/class-fs-logger.php

@@ -691,5 +695,38 @@ public static function get_logs_download_url( $filename = '' ) {
 			return rtrim( $upload_dir['url'], '/' ) . $filename;
 		}

+		/**
+		 * @param $file_path
+		 * @param $query_results


@DanieleAlessandra Parameter types are missing.

fajardoleo · 2024-11-28T14:36:44Z

includes/class-fs-logger.php

+				return false;
+			}
+
+			$content = implode( "\t", $headers ) . "\n";


@DanieleAlessandra The headers are duplicated by the foreach() loop below.

fajardoleo · 2024-12-04T16:24:55Z

templates/debug.php

+        } );
+    } );
+</script>
+<style>


@DanieleAlessandra It seems that the styles here should be in the assets/sass/admin/debug.scss file.

fajardoleo · 2024-12-04T16:31:23Z

templates/debug.php

@@ -284,7 +288,12 @@ function stopCountdownManually() {
        <?php endforeach ?>
    </tbody>
 </table>
-<h2><?php fs_esc_html_echo_x_inline( 'SDK Versions', 'as software development kit versions', 'sdk-versions' ) ?></h2>
+<h2>
+    <button class="toggle-table" aria-expanded="true">


@DanieleAlessandra I see that this is duplicated in multiple places, so I suggest storing it in a variable and then doing something like <?php echo $toggle_button ?>.

Also, the class should have the fs- prefix. It could be something like fs-debug-table-toggle-button and fs-debug-table-toggle-icon.

fajardoleo · 2024-12-04T16:41:39Z

templates/debug.php

+                button.find( '.caret-icon' ).text( isExpanded ? '▼' : '▲' );
+                table.css( {
+                    display  : isExpanded ? 'table' : 'block',
+                    maxHeight: isExpanded ? 'auto' : '0'


@DanieleAlessandra Maybe also add borderBottomWidth: isExpanded ? '1px' : '0',.

fajardoleo · 2024-12-04T16:45:47Z

templates/debug.php

@@ -714,6 +748,15 @@ function stopCountdownManually() {
        </table>
    <?php endif ?>
 <?php endforeach ?>
+<?php
+	$page_params = array(


@DanieleAlessandra, I see that the previous line uses spaces, not tabs.

fajardoleo · 2024-12-04T16:54:17Z

templates/debug.php

+		'is_open'        => false,
+	);
+
+	fs_require_template( 'debug/scheduled-crons.php', $page_params );


@DanieleAlessandra Regarding the duplicated <button class="toggle-table" aria-expanded="true">, if it's stored in a variable, then it can also be included in $page_params.

Add: Scheduled Crons; Add: Collapsible tables Fix: Export logs in CSV PHP compatibility Formatting [debug page] Fixed code style, Fixed duplicated header in exported file Changed toggle script from vanilla JS to jQuery Toggle button in a variable, class names Added toggle button factory function Added border width Moved css snippet to debug.scss

DanieleAlessandra requested a review from swashata October 11, 2024 08:48

fajardoleo reviewed Nov 28, 2024

View reviewed changes

templates/debug.php Show resolved Hide resolved

fajardoleo reviewed Nov 28, 2024

View reviewed changes

fajardoleo reviewed Dec 4, 2024

View reviewed changes

DanieleAlessandra force-pushed the feature/daniele/enrich-debug-page branch 2 times, most recently from 90aadc3 to 5a8a3de Compare December 4, 2024 20:02

DanieleAlessandra and others added 2 commits December 11, 2024 14:50

[debug-page] [code] Enhancements

65fc86c

DanieleAlessandra force-pushed the feature/daniele/enrich-debug-page branch from 0857469 to 65fc86c Compare December 11, 2024 13:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/daniele/enrich debug page #748

Feature/daniele/enrich debug page #748

DanieleAlessandra commented Oct 11, 2024

fajardoleo Nov 28, 2024

fajardoleo Nov 28, 2024

fajardoleo Nov 28, 2024

fajardoleo Nov 28, 2024

fajardoleo Nov 28, 2024

fajardoleo Nov 28, 2024 •

edited

Loading

fajardoleo Nov 28, 2024

fajardoleo Nov 28, 2024

DanieleAlessandra Dec 2, 2024

fajardoleo Nov 28, 2024 •

edited

Loading

fajardoleo Nov 28, 2024

fajardoleo Nov 28, 2024

fajardoleo Dec 4, 2024

fajardoleo Dec 4, 2024 •

edited

Loading

fajardoleo Dec 4, 2024

fajardoleo Dec 4, 2024

fajardoleo Dec 4, 2024

Feature/daniele/enrich debug page #748

Are you sure you want to change the base?

Feature/daniele/enrich debug page #748

Conversation

DanieleAlessandra commented Oct 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fajardoleo Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fajardoleo Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fajardoleo Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fajardoleo Nov 28, 2024 •

edited

Loading

fajardoleo Nov 28, 2024 •

edited

Loading

fajardoleo Dec 4, 2024 •

edited

Loading