-
-
Notifications
You must be signed in to change notification settings - Fork 932
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
The rubygem partial displays the latest release date #4476
base: master
Are you sure you want to change the base?
The rubygem partial displays the latest release date #4476
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why this change is needed? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
(rubygem.latest_version || rubygem.versions.last)&.number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not 100% sure what to do with config/locales/en.yml to get the "released" text displayed.
Took a look at
"- #{nice_date_for(version.authored_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. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! |
82f3095
to
e7b9799
Compare
Hello again @heyapricot and sorry for long delay on this review. I did some quick testing and it looks good. The only problem I have noticed is for pre-relase gems only |
Apologies for missing this message. Just caught it on my notifications while searching for other stuff. I'll follow the recommendation of checking the other method's logic and will try to work on this over the weekend |
Happy to review once ready @heyapricot! |
b3f10e3
to
e1041ca
Compare
e1041ca
to
81b2d90
Compare
@simi Comment addressed! Here is a brief demo |
This PR closes #2993. Let me know what you think!
Update: It closes #4470 as well