Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor changes to Zone11 msgids #1348

Merged

Conversation

marc-vanderwal
Copy link
Contributor

Purpose

This PR alters the msgids for Zone11 slightly.

Context

See #1328.

Changes

Small changes to the messages for Zone11.

How to test this PR

Compare the new messages with zonemaster/zonemaster#1264.

Do not refer to “SPF version 1” but just “SPF” because the former is
clunky and, until there is a new incompatible version of Sender Policy
Framework around, unnecessary.

Bring the msgids a bit closer in line to the specification.
@tgreenx tgreenx added this to the v2024.1 milestone May 16, 2024
@tgreenx tgreenx added the A-TestCase Area: Test case specification or implementation of test case label May 16, 2024
tgreenx
tgreenx previously approved these changes May 21, 2024
Copy link
Contributor

@tgreenx tgreenx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I suggest to wait for zonemaster/zonemaster#1264 to be completed first before merging.

},
Z11_SPF1_SYNTAX_ERROR => sub {
__x # ZONE:Z11_SPF1_SYNTAX_ERROR
'The SPF version 1 policy has a syntax error. Policy retrieved from the following nameservers: {ns_ip_list}.', @_;
'The zone’s SPF policy has a syntax error. Policy retrieved from the following nameservers: {ns_ip_list}.', @_;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be strict, the policy is a feature of a name (mail domain) not a zone. In a zone you could have multiple SPF policies for different mail domains, can't you. E.g. in the zone test.xa you could have one for afnic.test.xa and another for iis.test.xa given that those are used for mail addresses, e.g. info@afnic.test.xa and info@iis.test.xa, respectively. Zonemaster only tests the SPF policy of the mail domain equal to the zone apex, but that is still just a name. What do you say about:

The SPF policy of the zone apex has a syntax error. Policy retrieved from the following nameservers: {ns_ip_list}.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent suggestion. I’ll amend the msgid on both sides.

share/modules.txt Outdated Show resolved Hide resolved
matsduf
matsduf previously approved these changes May 22, 2024
matsduf
matsduf previously approved these changes May 24, 2024
lib/Zonemaster/Engine/Test/Zone.pm Outdated Show resolved Hide resolved
lib/Zonemaster/Engine/Test/Zone.pm Outdated Show resolved Hide resolved
lib/Zonemaster/Engine/Test/Zone.pm Outdated Show resolved Hide resolved
@tgreenx
Copy link
Contributor

tgreenx commented Jun 11, 2024

@marc-vanderwal I just merged zonemaster/zonemaster#1264. If you approve of my aforementioned changes in this PR, then you can merge it.

Technically, any domain, not just zones, can have SPF policies.
Zonemaster will only fetch the domain under test’s SPF policy. This
means that language referring to “the zone’s SPF policy” is technically
incorrect.

This commit updates the following messages tags to remove such language:

 * Z11_INCONSISTENT_SPF_POLICIES;
 * Z11_NO_SPF_FOUND;
 * Z11_SPF1_SYNTAX_ERROR;
 * Z11_SPF1_SYNTAX_OK.

The three last tags in the list get a new argument, {domain}, which is
necessary for the msgid to stay meaningful and user-friendly. The
alternative would be to say something like “the SPF policy at the zone’s
apex”, but “the SPF policy at domain.example” is much better. This is,
however, a change that should ideally be addressed by a database
migration script so that old tests stay meaningful.
According to Locale::TextDomain, __x() is only meant for translatable
strings with arguments in them. If those strings do not have any
arguments, calling __() is sufficient.
@tgreenx tgreenx requested a review from matsduf June 12, 2024 07:59
@tgreenx tgreenx merged commit 7e9d895 into zonemaster:develop Jun 12, 2024
3 checks passed
@tgreenx tgreenx added the V-Patch Versioning: The change gives an update of patch in version. label Jun 12, 2024
@mattias-p mattias-p self-assigned this Jun 25, 2024
@mattias-p
Copy link
Member

Release testing for v2024.1

I've verified that the new text is reported for a couple of the updated messages.

@mattias-p mattias-p added the S-ReleaseTested Status: The PR has been successfully tested in release testing label Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-TestCase Area: Test case specification or implementation of test case S-ReleaseTested Status: The PR has been successfully tested in release testing V-Patch Versioning: The change gives an update of patch in version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implemented msgid for Zone11 do not match the messages in specification
4 participants