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.19%. Comparing base (a86b845) to head (ecb6a85).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5237      +/-   ##
==========================================
- Coverage   96.85%   94.19%   -2.67%     
==========================================
  Files         456      456              
  Lines        9517     9578      +61     
==========================================
- Hits         9218     9022     -196     
- Misses        299      556     +257     

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

[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?