Add predefined date ranges, and UI tooltips

[neekfenwick]

These are changes from the commit scottcwilson/Email_Archive_Manager@1053e6c which seems to have been missed as my PR there was incorporated into this plugin.

The structure here is quite different (in a good way) so I've had to get slightly creative but it's basically the same stuff.

One thing I'm not clear on is the styling of the radio buttons seems to have some space on their left edge that wasn't there before. It's late here, I'm going to leave this here and will come back in the morning.

By the way don't worry about the datepicker not having an icon, it's my custom fontawesome font missing that glyph, I'll fix that locally.

[drbyte]

Thanks for updating! I'll dig into reviewing this afternoon.

I also note that the SELECT includes `'errorinfo' even if it's not being examined, I left that out on purpose because the docs say that a TEXT column requires more work to materialise and calculate, so it seemed wise to omit it from the SELECT list if not required.

I'd opted to include it "anyway". because I figured most error messages aren't actually going to be monstrous. And errors won't happen regularly. Plus we're only listing like max 25 or so records at a time, so it's not gonna blow out RAM.
That's what my thinking was anyway.
And I'd built the search-text to also search the errors field, cuz chances are whatever you're searching about an error wouldn't be in the email, and what you're searching in an email wouldn't be in an error, most of the time, so it shouldn't taint the results badly and simplified the UI slightly.
Again, that's what I'd been thinking.

One thing I'm not clear on is the styling of the radio buttons seems to have some space on their left edge that wasn't there before.

I'll have to model your code to see what you mean.

[drbyte]

Food for thought:

 define('SUBJECT_SIZE_LIMIT',  200);
 define('MESSAGE_SIZE_LIMIT',  550);

Will storeowners need to change these at all?

I don't want to create configuration table entries for these settings. Having them hardcoded as constants was kinda okay when the plugin was something people drop into the core app directory's files, to manually edit those values there.
But with an encapsulated plugin like this it's a bit clunky to have people edit the php file directly.

So, my question is: do these values "need" to be user-editable? Or are these defaults "fine"?
If they need to be editable, should we build a couple input fields in a separate form that's also on the main page, and sets an override into the session?
Or am I overthinking things?

I'm inclined to leave it as-is: IMO these are reasonable defaults. But input welcome.

[neekfenwick]

I'm inclined to leave it as-is: IMO these are reasonable defaults. But input welcome.

Fair points but I think you're overthinking. I don't know where these constants came from exactly, I did pop the subject one up from 25 to 200 to let the CSS ellipsis rule do more heavy lifting. Building something to make them more editable seems like overkill.

[drbyte]

And thanks for catching some of my missed updates to echo syntax. 👍

[neekfenwick]

Do you have an opinion on removing the Archive ID column and replacing it with a tooltip or info circle or something with the archive id value available? e.g.: in Date Sent column:
<td>22/05/2024 01:22:33 <i class="fa-circle-info" title="Archive ID: 1020304">

I feel most store owners won't care about the ID value and it just takes up screen space. Developers needing to access a database record by ID can still get to it. Another trick often used it to put it in an expando attribute like <td data-archive-id="1020304"> but I see no need for programmatic access to the number at this point.

[drbyte]

I'm okay with preserving the screen space by hiding the ID behind a tooltip.

I'm mindful that the ID shows when viewing the actual email full-screen via either of the text/html buttons in the right column ... and sometimes when parsing through an on-screen list it's nice to correlate those IDs from individual pages to the "grid".
But I dunno how important it'll be to people. The tooltip is a reasonable compromise.

[neekfenwick]

I notice email_archive table does not seem to have an index on date_sent, on my prod DB this means an email search even over a short date range scans over 1M rows. CREATE INDEX idx_email_archive ON email_archive (date_sent) means a scan over this year only examines about 170K rows. The search queries from this plugin would benefit hugely from such an index.

I added one years ago to help my troubleshooting so I might be confusing myself but I see nothing in the install/upgrade SQL that adds an index. However a recent 158 database I installed has two indexes idx_email_to_address_zen and idx_module_zen and I can't see where these are created during install.

If you agree, how would you add this index? Core ZC? And/or a patch-in in this addon like is done for the errorinfo column?

[drbyte]

https://github.com/zencart/zencart/blob/56665f8e45d46dbc503b73f9d1e9a768990448fa/zc_install/sql/install/mysql_zencart.sql#L756-L758

I support a PR to core ZC to add the date_sent index, using our usual naming convention of idx_ prefix and _zen suffix with the field names listed in between (I added the word "email" as well in this case, for verbosity):
installer:
KEY idx_email_date_sent_zen (date_sent)
upgrader:
CREATE INDEX idx_email_date_sent_zen ON email_archive (date_sent)

It could be added to the ScriptedInstaller for this plugin as well (again, preferably a separate PR for it), but would have to first check whether an index by that same name already exists, else install/upgrade will fail.
Code similar to this function could be used to do that check (but the following code only exists in master right now, so won't be in ZC core until v2.1.0):
https://github.com/zencart/zencart/blob/a068ff7007e9989eaa0e5f824c05fad18be2f172/includes/classes/sniffer.php#L126-L135

[neekfenwick]

I support a PR to core ZC to add the date_sent index

Sounds great, could you do the core ZC please, as I'm not nearly as familiar as you. I can probably handle this addon changes. If the core "index exists" helper fn does not exist pre v2.1 in core it'll have to have its own in-house fn for back-compat, I guess I'll add that.

I missed the index creation in core 'cause I almost never do it in the CREATE TABLE statement, so didn't even bother looking there :) Weird that my text search didn't find it. Whatever, doh!

[drbyte]

I'd forgotten: We usually don't use CREATE INDEX, but rather ALTER TABLE ... ADD INDEX, like this:
ALTER TABLE admin ADD INDEX idx_admin_profile_zen (admin_profile);

[drbyte]

Done: Index added to v2.1.0

[neekfenwick]

See this latest commit, with recently discussed changes. Looks pretty good. Possible merge candidate, I think it's up to date with main branch.

[neekfenwick]

Hang on, I'm going to make the default date range shorter, like 30 days, no shop is going to want to request their entire history on initial page load.

[neekfenwick]

Done.

[neekfenwick]

Er. I've left the CSS kind of half cocked. Was trying to get the columns to be a fixed width but it didn't work because <table>s suck :P will fix.

[drbyte]

Thanks @neekfenwick !