Build trusted publisher filter

[justinddaniel]

No description provided.

[simi]

Doesn't trusted_publisher? already return true/false? Is there any reason for explicit ? true: false?

[justinddaniel]

The safe navigator means a nil value can be returned if the version has no pusher_api_key.

[simi]

Would pusher_api_key? && pusher_api_key.trusted_publisher? work also?

[justinddaniel]

No, but pusher_api_key.present? && pusher_api_key.trusted_publisher? would work. It is a matter of preference here.

[simi]

Or maybe pusher_api_key&.trusted_publisher? || false. Yes, just a nitpick indeed.

[martinemde]

Am I the only one that prefers !! to force boolean?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.33%. Comparing base (46ff0a3) to head (8bf1b85).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5237      +/-   ##
==========================================
- Coverage   97.11%   94.33%   -2.79%     
==========================================
  Files         454      454              
  Lines        9441     9502      +61     
==========================================
- Hits         9169     8964     -205     
- Misses        272      538     +266     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[justinddaniel]

Linked issue: #4807

[simi]

CI seems unhappy about this test.

rubygems.org/test/models/pusher_test.rb

Lines 632 to 659 in 1143eba

	expected_response = {
	"name" => "gemsgemsgems",
	"downloads" => 0,
	"version" => "0.1.1",
	"version_downloads" => 0,
	"platform" => "ruby",
	"authors" => "Joe User",
	"info" => "Some awesome gem",
	"licenses" => "MIT",
	"metadata" => { "foo" => "bar" },
	"sha" => "b5d4045c3f466fa91fe2cc6abe79232a1a57cdf104f7a26e716e0a1e2789df78",
	"project_uri" => "http://localhost/gems/gemsgemsgems",
	"gem_uri" => "http://localhost/gems/gemsgemsgems-0.1.1.gem",
	"homepage_uri" => "http://example.com",
	"wiki_uri" => "http://example.com",
	"documentation_uri" => "http://example.com",
	"mailing_list_uri" => "http://example.com",
	"source_code_uri" => "http://example.com",
	"bug_tracker_uri" => "http://example.com",
	"changelog_uri" => nil,
	"funding_uri" => nil,
	"yanked" => false,
	"summary" => "old summary",
	"description" => "Some awesome gem",
	"updated" => "2016-07-04T00:00:00.000Z",
	"dependencies" => { "development" => [], "runtime" => [] },
	"suggest" => { "input" => "gemsgemsgems", "weight" => 0, "contexts" => { "yanked" => false } }
	}

Should be as simple fix as adding this new key in to the tested hash.

[simi]

Does this mapping change need full re-indexing?

[justinddaniel]

Good question. I would not think so, since there is no change to the database source schema, but I am not sure. It is my first time committing to this repo and I do not have full context, so I will wait for others to weigh in.

[colby-swandale]

Any new mappings do require a reindex, which is easy to do but needs some coordination when deployed.

[martinemde]

@justinddaniel Thank you for contributing this! Pleasure working with you today.

[martinemde]

The coverage fail can be ignored. What's the harm in merging this without reindexing? Will it just ignore the option until we do? If that's all, can we merge and then run the reindexing at our convenience?