diff --git a/batch_test.go b/batch_test.go index d5c8400..c6ffc09 100644 --- a/batch_test.go +++ b/batch_test.go @@ -70,6 +70,8 @@ func TestBatch(t *testing.T) { token := env.admin(t, true) env.createPage(t, token, map[string]any{"title": "Page A", "slug": "a", "excerpt": "About A", "published": true}) env.createPage(t, token, map[string]any{"title": "Page B", "slug": "b", "excerpt": "About B", "published": true}) + // noindex_published needs the address in a sitemap file. + env.rebuildSitemap(t) agent := env.principal(t, env.admin(t, false, "golem15.seo.manage")) changes := []any{ diff --git a/classes/batch.go b/classes/batch.go index 478847a..bfe007e 100644 --- a/classes/batch.go +++ b/classes/batch.go @@ -290,8 +290,9 @@ func (s *Service) runBatch(ctx context.Context, opts BatchOptions, build func(ct } res.Changes = append(res.Changes, item) } + listed := &listedOnce{svc: s} for i := range res.Changes { - late, err := s.batchConflicts(ctx, tx, i, changes[i], res.Changes[i]) + late, err := s.batchConflicts(ctx, tx, listed, i, changes[i], res.Changes[i]) if err != nil { return err } @@ -452,12 +453,34 @@ func (s *Service) applyChange(ctx context.Context, tx *gorm.DB, index int, ch Ba return errUnknownOp } +// listedOnce reads the addresses the sitemaps list once per batch. +type listedOnce struct { + svc *Service + read bool + ok bool + listed map[string]bool +} + +// has reports whether the sitemaps list address; ok is false without the +// sitemap plugin's Listing. +func (l *listedOnce) has(ctx context.Context, address string) (listed, ok bool, err error) { + if !l.read { + l.listed, l.ok, err = l.svc.SitemapListed(context.WithoutCancel(ctx)) + if err != nil { + return false, false, err + } + l.read = true + } + return l.listed[address], l.ok, nil +} + // batchConflicts are the warnings that depend on the whole batch, checked // once every change is in place: noindex on the address of a published -// record (the records the sitemaps list; Phase 33's sitemap filter -// refines it) and a canonical pointing to a redirected or noindex -// address. -func (s *Service) batchConflicts(ctx context.Context, tx *gorm.DB, index int, ch BatchChange, item BatchItem) ([]Conflict, error) { +// record that a sitemap file lists now (read through contract.Listing, so +// an address the sitemaps already leave out, such as one of a content +// type switched off, is no conflict; D-12, D-24), and a canonical pointing +// to a redirected or noindex address. +func (s *Service) batchConflicts(ctx context.Context, tx *gorm.DB, listed *listedOnce, index int, ch BatchChange, item BatchItem) ([]Conflict, error) { if item.Error != "" { return nil, nil } @@ -490,7 +513,15 @@ func (s *Service) batchConflicts(ctx context.Context, tx *gorm.DB, index int, ch if err != nil && !errors.Is(err, ErrBadURL) { return nil, err } - if err == nil && owner.WrittenTo == WrittenToRecord && owner.Published { + inSitemap := err == nil && owner.WrittenTo == WrittenToRecord && owner.Published + if inSitemap { + has, ok, lerr := listed.has(ctx, owner.URL) + if lerr != nil { + return nil, lerr + } + inSitemap = !ok || has + } + if inSitemap { out = append(out, Conflict{Change: index, Kind: ConflictNoindexPublished, URL: owner.URL, Message: s.PhraseCtx(ctx, "golem15.seo::lang.api.conflict_noindex_published", map[string]string{"url": owner.URL})}) } diff --git a/classes/issues.go b/classes/issues.go index 4b09cd0..4db8a1b 100644 --- a/classes/issues.go +++ b/classes/issues.go @@ -21,6 +21,15 @@ const ( IssueCanonicalNoindex = "canonical_noindex" IssueCanonicalRedirected = "canonical_redirected" IssueNoindexBlocked = "noindex_blocked" + // IssueSitemapBlocked is a sitemap address the production robots.txt + // keeps Googlebot from (D-24): an error, since the sitemap asks for a + // page the crawler may not fetch. + IssueSitemapBlocked = "sitemap_blocked" +) + +// The severities an issue may name. +const ( + SeverityError = "error" ) // Issue is one finding of the issue report: the address, the kind, the @@ -31,6 +40,9 @@ type Issue struct { Locale string `json:"locale,omitempty"` Details map[string]any `json:"details,omitempty"` Tool string `json:"tool"` + // Severity is set for kinds that are errors (sitemap_blocked); "" for + // the others. + Severity string `json:"severity,omitempty"` } // The tools the issues point to (pier prefixes them with seo_). @@ -71,6 +83,14 @@ func (s *Service) Issues(ctx context.Context) ([]Issue, error) { if data.noindex, err = noindexAddresses(db); err != nil { return nil, err } + listed, _, err := s.SitemapListed(ctx) + if err != nil { + return nil, err + } + for address := range listed { + data.listed = append(data.listed, address) + } + sort.Strings(data.listed) return s.issuesOf(ctx, data, s.canonicalProblem) } @@ -85,6 +105,8 @@ type issueData struct { redirects []models.Redirect robots models.Robots noindex []string + // listed are the addresses the sitemaps list (path with query). + listed []string } // issuesOf builds the report from data; canonical classifies a canonical @@ -228,10 +250,22 @@ func (s *Service) issuesOf(ctx context.Context, data issueData, canonical func(c } // Noindex addresses robots.txt blocks. + file := ComposeRobots(data.robots.Content, data.robots.Bots, s.SitemapLines(ctx)) for _, u := range s.blockedNoindex(data.noindex, data.robots.Content, data.robots.Bots) { out = append(out, Issue{Kind: IssueNoindexBlocked, URL: u, Tool: toolUpdateRobots}) } + // Sitemap addresses the production robots.txt blocks for Googlebot. + for _, u := range data.listed { + test := s.testRobotsFile(file, data.robots.Content, "Googlebot", u) + if test.Allowed { + continue + } + out = append(out, Issue{Kind: IssueSitemapBlocked, URL: u, Tool: toolUpdateRobots, Severity: SeverityError, Details: map[string]any{ + "line": test.Line, "rule": test.Rule, + }}) + } + sort.SliceStable(out, func(i, j int) bool { if out[i].Kind != out[j].Kind { return out[i].Kind < out[j].Kind diff --git a/classes/partials.go b/classes/partials.go index a7d53a0..8f84158 100644 --- a/classes/partials.go +++ b/classes/partials.go @@ -8,6 +8,7 @@ import ( "strings" "git.golem15.com/golem15/sm-seo-plugin/models" + "git.golem15.com/golem15/sm-sitemap-plugin/contract" "gorm.io/gorm" ) @@ -15,10 +16,12 @@ import ( // the request locale), never a GORM model; html/template escapes every // value. -// Callout is one P1 warning. +// Callout is one P1 callout: a warning, or a plain note when Plain is +// set (the sitemap status). type Callout struct { Title string Text string + Plain bool } // StatusView is the P1 seo_status view model. @@ -202,9 +205,33 @@ func (s *Service) RecordStatus(ctx context.Context, key, id string) (StatusView, } } } + exclusions, err := s.RecordSitemapExclusions(ctx, key, id) + if err != nil { + return view, err + } + for _, x := range exclusions { + key, ok := excludedTexts[x.Reason] + if !ok { + continue + } + view.Callouts = append(view.Callouts, Callout{ + Title: s.PhraseCtx(ctx, "golem15.seo::lang.status.excluded_title", nil), + Text: s.PhraseCtx(ctx, key, map[string]string{"url": x.Target}), + Plain: true, + }) + } return view, nil } +// excludedTexts are the P1 texts of the sitemap exclusion reasons (UI-SPEC +// C16). +var excludedTexts = map[string]string{ + contract.ReasonNoindex: "golem15.seo::lang.status.excluded_noindex", + contract.ReasonCanonical: "golem15.seo::lang.status.excluded_canonical", + contract.ReasonRedirect: "golem15.seo::lang.status.excluded_redirect", + contract.ReasonType: "golem15.seo::lang.status.excluded_type", +} + // ruleCallout is the override callout of the first rule that sets a value. func (s *Service) ruleCallout(ctx context.Context, rules []models.Rule) (Callout, bool) { for _, r := range rules { diff --git a/classes/robots.go b/classes/robots.go index 9a39f66..03f82dc 100644 --- a/classes/robots.go +++ b/classes/robots.go @@ -205,7 +205,12 @@ type RobotsTest struct { // grobotstxt, Google's matcher (D-22, D-99): longest match, Allow wins a // tie, * and $. func (s *Service) TestRobots(content string, bots models.BotSwitches, agent, path string) RobotsTest { - file := ComposeRobots(content, bots, s.SitemapLines(context.Background())) + return s.testRobotsFile(ComposeRobots(content, bots, s.SitemapLines(context.Background())), content, agent, path) +} + +// testRobotsFile is TestRobots on a composed file; content is the editor's +// part of it (the generated bot rules follow it). +func (s *Service) testRobotsFile(file, content, agent, path string) RobotsTest { uri := AbsoluteURL(s.SiteURL(), path) if s.SiteURL() == "" { uri = "http://localhost" + path @@ -269,8 +274,12 @@ func (s *Service) BlockedNoindex(db *gorm.DB, content string, bots models.BotSwi // the production file composed from content and bots. func (s *Service) blockedNoindex(urls []string, content string, bots models.BotSwitches) []string { var out []string + if len(urls) == 0 { + return out + } + file := ComposeRobots(content, bots, s.SitemapLines(context.Background())) for _, u := range urls { - if !s.TestRobots(content, bots, "Googlebot", u).Allowed { + if !s.testRobotsFile(file, content, "Googlebot", u).Allowed { out = append(out, u) } } diff --git a/classes/sitemap_filter.go b/classes/sitemap_filter.go index b4b671a..89eb6d9 100644 --- a/classes/sitemap_filter.go +++ b/classes/sitemap_filter.go @@ -132,23 +132,31 @@ func (s *Service) newFilterBatch(ctx context.Context) (*filterBatch, error) { }, nil } -// decide is the decision of one candidate: a redirected address first (it -// is no page), then noindex, then a canonical URL elsewhere. +// decide is the decision of one candidate. func (b *filterBatch) decide(ctx context.Context, c contract.Candidate) (contract.Decision, error) { + d, _, err := b.decideTarget(ctx, c) + return d, err +} + +// decideTarget is the decision of one candidate and, for a canonical or a +// redirect, the address the sitemaps would list instead: a redirected +// address first (it is no page), then noindex, then a canonical URL +// elsewhere. +func (b *filterBatch) decideTarget(ctx context.Context, c contract.Candidate) (contract.Decision, string, error) { path, query, rawQuery, ok := b.candidatePath(c.URL) if !ok { - return contract.Decision{}, nil + return contract.Decision{}, "", nil } norm, changed := Normalize(&url.URL{Path: path, RawQuery: rawQuery}, b.opts) if changed { - return contract.Decision{Exclude: true, Reason: contract.ReasonRedirect}, nil + return contract.Decision{Exclude: true, Reason: contract.ReasonRedirect}, joinQuery(norm.Path, norm.RawQuery), nil } chain, err := b.set.Follow(b.site, norm.Path, norm.RawQuery, MaxRedirectHops) if err != nil { - return contract.Decision{}, err + return contract.Decision{}, "", err } if len(chain.Hops) > 0 { - return contract.Decision{Exclude: true, Reason: contract.ReasonRedirect}, nil + return contract.Decision{Exclude: true, Reason: contract.ReasonRedirect}, displayURL(b.site, chain.Final), nil } key := "" var match *pact.ContentMatchResult @@ -160,20 +168,121 @@ func (b *filterBatch) decide(ctx context.Context, c contract.Candidate) (contrac } res, err := b.resolver.resolveAt(ctx, b.db, b.settings, path, query, rawQuery, key, match, nil, true) if err != nil { - return contract.Decision{}, err + return contract.Decision{}, "", err } if res.SEO == nil { - return contract.Decision{}, nil + return contract.Decision{}, "", nil } if r, err := ParseRobots(res.SEO.Robots); err == nil && r.Index == "noindex" { - return contract.Decision{Exclude: true, Reason: contract.ReasonNoindex}, nil + return contract.Decision{Exclude: true, Reason: contract.ReasonNoindex}, "", nil } if !sameAddress(res.SEO.Canonical, res.URL) { - return contract.Decision{Exclude: true, Reason: contract.ReasonCanonical}, nil + return contract.Decision{Exclude: true, Reason: contract.ReasonCanonical}, displayURL(b.site, res.SEO.Canonical), nil } - return contract.Decision{}, nil + return contract.Decision{}, "", nil } +// SitemapExclusion is why a record's address is left out of the sitemaps: +// a contract reason and, for canonical and redirect, the address listed +// instead. +type SitemapExclusion struct { + Locale string + Reason string + Target string +} + +// RecordSitemapExclusions lists why the sitemaps leave out the addresses +// of record id of content type key, one per reason (the first locale that +// has it): the type switched off in the sitemap settings (read through +// contract.Listing), else the SEO plugin's own decision on each address +// (noindex, canonical elsewhere, redirect). Nothing for an unpublished +// record: the sitemaps never list it. +func (s *Service) RecordSitemapExclusions(ctx context.Context, key, id string) ([]SitemapExclusion, error) { + reg := s.Registry() + if reg == nil { + return nil, nil + } + rec, err := reg.Describe(ctx, key, id, s.UnprefixedLocale()) + if err != nil || rec == nil || !rec.Published { + return nil, err + } + if l := s.listing(); l != nil { + st, err := l.Settings(ctx) + if err != nil { + return nil, err + } + for _, off := range st.TypesOff { + if off == key { + return []SitemapExclusion{{Reason: contract.ReasonType}}, nil + } + } + } + b, err := s.newFilterBatch(ctx) + if err != nil { + return nil, err + } + var out []SitemapExclusion + seen := map[string]bool{} + for _, l := range rec.Locales { + if strings.TrimSpace(l.URL) == "" { + continue + } + d, target, err := b.decideTarget(ctx, contract.Candidate{TypeKey: key, RecordID: id, Locale: l.Locale, URL: l.URL}) + if err != nil { + return nil, err + } + if d.Exclude && !seen[d.Reason] { + seen[d.Reason] = true + out = append(out, SitemapExclusion{Locale: l.Locale, Reason: d.Reason, Target: target}) + } + } + return out, nil +} + +// SitemapListed is the set of addresses the sitemaps list, read through +// contract.Listing: their paths with the query, as the owner of a URL +// names them. ok is false without a published Listing. At most +// MaxListedAddresses are read. +func (s *Service) SitemapListed(ctx context.Context) (map[string]bool, bool, error) { + l := s.listing() + if l == nil { + return nil, false, nil + } + st, err := l.Settings(ctx) + if err != nil { + return nil, false, err + } + out := map[string]bool{} + read := 0 + for _, g := range st.Groups { + after := "" + for read < MaxListedAddresses { + page, err := l.Entries(ctx, contract.EntriesQuery{Group: g.Key, After: after, Limit: 1000}) + if err != nil { + return nil, false, err + } + for _, e := range page.Entries { + read++ + if u, err := url.Parse(e.URL); err == nil { + path := u.Path + if path == "" { + path = "/" + } + out[joinQuery(path, u.RawQuery)] = true + } + } + if page.Next == "" || page.Next == after { + break + } + after = page.Next + } + } + return out, true, nil +} + +// MaxListedAddresses caps the sitemap addresses SitemapListed reads. +const MaxListedAddresses = 100000 + // candidatePath turns a candidate's address (a path, or an absolute URL on // the site or its www twin) into the decoded path and its query; ok is // false for an address the plugin cannot judge (another host, malformed). diff --git a/controllers/api/issues.go b/controllers/api/issues.go index ad6a32a..9e254df 100644 --- a/controllers/api/issues.go +++ b/controllers/api/issues.go @@ -9,7 +9,7 @@ import ( // ListIssuesInput is the query of GET /_seo/api/v1/issues. type ListIssuesInput struct { - Kind string `json:"kind,omitempty" query:"kind" jsonschema:"one kind only: missing_title, missing_description, duplicate_title, duplicate_description, rule_overrides_record, redirect_chain, canonical_noindex, canonical_redirected or noindex_blocked"` + Kind string `json:"kind,omitempty" query:"kind" jsonschema:"one kind only: missing_title, missing_description, duplicate_title, duplicate_description, rule_overrides_record, redirect_chain, canonical_noindex, canonical_redirected, noindex_blocked or sitemap_blocked"` } // ListIssues serves GET /_seo/api/v1/issues (D-13, D-14 CMS part): the diff --git a/extension/_seo_status.htm b/extension/_seo_status.htm index de192cc..f9f7fc2 100644 --- a/extension/_seo_status.htm +++ b/extension/_seo_status.htm @@ -1,5 +1,5 @@ {{- range .Data.Callouts -}} -
{{ .Title }}
{{- if .Text }}{{ .Text }}
{{ end }}