Opened 13 years ago

Closed 13 years ago

#270 closed defect (fixed)

[PATCH] send-quarantine-digests.pl bugs & enhancements

Reported by: erik@… Owned by: dmorton
Priority: high Milestone: 1.0.1
Component: Perl scripts Version: 1.0.0
Severity: normal Keywords: digests, refactor, performance, empty, multiple
Cc:

Description

Normally, I'd break this into 5 tickets, but I have a patch that covers all 5:

  1. Bugs
    1. The value of $at_least_one is not reset when starting to process a new user, so that once this value is set, all subsequent users generate mail.
    2. Because of variable scoping, if the untainting of a variable fails, it retains its previous value (notably, $user_email and $subject)
    3. Within the report loop, $report_count is incremented, even when $sth->rows indicates that nothing was selected. This causes @{ $vars{'list'} } to have holes.
  2. Enhancements
    1. Repeatedly PREPARE'ing the same SQL statement over & over is wasteful and unecessarily taxing on DB resources, which might be at a premium. A better solution is to prepare once and execute as many times as is necessary. Some slight refactoring of this code was necessary to facilitate that.
    2. get_config_values() reads the same (unchanging) values from the database every time it is called. Since it is called every time a token is generated, it's called for each line in every report. Adding a cache of these values for the lifetime of this script cuts down on DB overhead.
  3. Other enhancements that might actually hamper acceptance of this patch
    1. Code Normalization
      1. Indentation — 4-space indent (no tabs) seemed to be the preference
      2. Block brackets — no prepended newline (only had a two exceptions)
    2. SQL
      1. Refactored to allow a single PREPARE statement for each statement used more than once.
      2. Now stored in HERE documents, to facilitate reading & editing
    3. Subroutines moved to the end
      1. Since they're using pre-PREPARE'd statement handles, it's less obtuse if we don't have to pre-declare the variables.
      2. It's also awkward to have them stuck in the middle of of the main code (between user-config & the rest of the "main" block.) Prototypes are left in that space to ensure proper usage.

Attachments (3)

send-quarantine-digests-refactor.patch (15.3 KB) - added by erik@… 13 years ago.
patch send...digests.pl bug fixes, refactoring per #270
send-quarantine-digests-refactor.2.patch (15.3 KB) - added by erik@… 13 years ago.
[IGNORE LAST BASED ON DEBUGGING] patch send...digests.pl bug fixes, refactoring per #270
send-quarantine-digests.pl (14.1 KB) - added by erik@… 13 years ago.
The new version of send-quarantine-digests.pl (based on 1.0.0)

Download all attachments as: .zip

Change History (6)

Changed 13 years ago by erik@…

patch send...digests.pl bug fixes, refactoring per #270

Changed 13 years ago by erik@…

[IGNORE LAST BASED ON DEBUGGING] patch send...digests.pl bug fixes, refactoring per #270

Changed 13 years ago by erik@…

The new version of send-quarantine-digests.pl (based on 1.0.0)

comment:1 Changed 13 years ago by dmorton

  • Milestone set to 1.0.1
  • Owner changed from rjl to dmorton
  • Priority changed from normal to high
  • Status changed from new to assigned

comment:2 Changed 13 years ago by dmorton

patched trunk in [940], pending more testing, can be merged into 1.0 branch...

comment:3 Changed 13 years ago by dmorton

  • Resolution set to fixed
  • Status changed from assigned to closed

small fix in [943], merged into 1.0 branch in [947]

Note: See TracTickets for help on using tickets.