tierpod / dmarc-report-converter

Convert dmarc reports from xml to human-readable formats
MIT License
223 stars 25 forks source link

Make AssetsPath available in external_template context #46

Closed moorereason closed 2 months ago

moorereason commented 3 months ago

When building a custom template using the external_template format, the .AssetsPath value is not available to the template engine context.

tierpod commented 3 months ago

Hello, thank you for the report.

Could you please explain this use case more detailed? IIRC, I haven't exported this variable for external template on purpose. I assume, if you use external_template (and you write this template) you can hardcode necessary assets path in this template.

moorereason commented 3 months ago

First, I've been looking for a simple tool like dmarc-report-converter that doesn't require a bunch of resources and infrastructure (preferably written in Go). I've setup an AWS SES sub-domain to deliver reports to an S3 bucket, which I pull down before running dmarc-report-converter. It's working great so far, so thank you for all of the hard work you've put into this project before I showed up complaining about things. 😆

My use case started with wanting to hack on the default template (htmlTmpl from consts.go). I copied the contents to a separate file and ran it. Right out of the gate, I got errors and had to define my own template variable and rewrite all references of .AssetsPath to $AssetsPath. This is when I opened this issue (in haste!).

But then I quickly realized that the default contexts are completely different. The external template's default context is just the pkg/dmarc.Report struct. So, I had to rewrite all of that, too (something like s/({{\s*\.)Report./$1/g).

I find all of that an annoying learning curve and a barrier to entry for newcomers (like me). I don't see the harm in exporting AssetsPath. If the user doesn't want to use it, they don't have to. IMHO, it would be more intuitive to have the same contexts in each template so that I could use the built-in templates as a guide.

Potential Fix

A potential backward-compatible fix could be something like:

type TmplCtx struct {
    AssetsPath      string
    Report          dmarc.Report

    // Deprecate these properties
    XMLName         xml.Name
    ReportMetadata  dmarc.ReportMetadata
    PolicyPublished dmarc.PolicyPublished
    Records         []dmarc.Record
    MessagesStats   dmarc.MessagesStats
}
tierpod commented 3 months ago

Thank you for explanation.

I understand your use case and it's completely valid. I agree that it would be better for new users to have consistent variables across all kind of templates (didn't think about it when I implemented external template).