The rubygem partial displays the latest release date #4476
Conversation
There was a problem hiding this comment.
Hello @heyapricot! Thanks for opening this PR. The overall idea seems fine to me. But the implementation would need few tweaks. Feel free to ping me if help is needed.
current screenshot
| @@ -3,9 +3,10 @@ class SearchesController < ApplicationController | |||
|
|
|||
| def show | |||
| return unless params[:query].is_a?(String) | |||
| @error_msg, @gems = ElasticSearcher.new(params[:query], page: @page).search | |||
| @error_msg, @elastic_results = ElasticSearcher.new(params[:query], page: @page).search | |||
There was a problem hiding this comment.
Can you explain why this change is needed? 🤔
There was a problem hiding this comment.
This was me circumventing an issue to get a working proof of concept 😬
@gems originally is assigned a collection of Hash-like objects like this:
<Searchkick::HashWrapper _id="15494" _index="rubygems-development_20240219203755949" _score=248.40211 _type="_doc" description="Ruby on Rails is a full-stack web framework optimized for programmer happiness and sustainable productivity. It encourages beautiful code by favoring convention over configuration." downloads=496344261 id="15494" name="rails" summary="Full-stack web application framework." version="7.1.3">
I couldn't figure out how to add the latest_release_date property to the HashWrapper (Took a look here for example)
What I did instead was map the id's from the search result collection, run a new query and use the actual Rubygem objects instead.
There was a problem hiding this comment.
Yup, AFAIK you can use version attribute on returned search result.
app/helpers/rubygems_helper.rb
Outdated
| @@ -161,6 +161,10 @@ def show_all_versions_link?(rubygem) | |||
| rubygem.versions_count > 5 || rubygem.yanked_versions? | |||
| end | |||
|
|
|||
| def latest_release_date(rubygem) | |||
| (rubygem.latest_version || rubygem.versions&.last)&.built_at&.strftime("%B %d, %Y") | |||
There was a problem hiding this comment.
Is there any case where rubygem.latest_version is nil (or false) but rubygem.versions.last is not? Also created_at should be used.
There was a problem hiding this comment.
I ran Rubygem.left_outer_joins(:versions).where(versions: { id: nil }) and it returned 79 results. All of them had the indexed property set to false (Not sure if that affects the search result eligibility).
I followed the pattern from this method:
rubygems.org/app/helpers/rubygems_helper.rb
Line 166 in 0c48ef3
There was a problem hiding this comment.
Ended up removing the latest_release_date method and using version_date_tag instead
app/views/rubygems/_rubygem.html.erb
Outdated
| <%= latest_version_number(rubygem) %> | ||
| </span> | ||
| <span class="gems__gem__latest__release__date"> | ||
| <%= "released #{latest_release_date(rubygem)}" %> |
There was a problem hiding this comment.
released should be moved into config/locales/en.yml. Would it be possible to reuse version_date_tag from VersionsHelper?
There was a problem hiding this comment.
Not 100% sure what to do with config/locales/en.yml to get the "released" text displayed.
Took a look at
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4476 +/- ##
==========================================
- Coverage 97.15% 96.88% -0.27%
==========================================
Files 391 391
Lines 8260 8261 +1
==========================================
- Hits 8025 8004 -21
- Misses 235 257 +22 ☔ View full report in Codecov by Sentry. |
heyapricot
force-pushed
the
add-latest-release-date-to-search-index
branch
from
d7c5d86
to
3941c33
Compare
app/views/rubygems/_rubygem.html.erb
Outdated
| </span> | ||
| <span class="gems__gem__latest__release__date"> | ||
| <%= version_date_tag(rubygem.latest_version) %> | ||
| </span> |
There was a problem hiding this comment.
Is this an acceptable appearance?
heyapricot
force-pushed
the
add-latest-release-date-to-search-index
branch
2 times, most recently
from
be24725
to
82f3095
Compare
|
@simi Submitted a different and hopefully simpler approach. updated entry so i decided to use that to display the release date when delivering search results. Previously you suggested using created_at but I wanted to show the approach before modifying the search_data method
I included a helper method Let me know what you think. Thanks! |
heyapricot
force-pushed
the
add-latest-release-date-to-search-index
branch
from
82f3095
to
e7b9799
Compare
This PR closes #2993. Let me know what you think!
Update: It closes #4470 as well