Skip to content
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

Use #latest_version when loading the profile page rubygems #4627

Merged

Conversation

dancristianb
Copy link
Contributor

@dancristianb dancristianb commented Apr 19, 2024

Fixes #4624

This PR targets fixing the profile page gem versions issue described in #4624. I've pulled down a copy of the data using the instructions in https://github.com/rubygems/rubygems.org/blob/master/CONTRIBUTING.md#getting-the-data-dumps and tried it out locally. The fix seems to work correctly πŸ‘

I'm not sure if this is the best approach though πŸ€” I also found that this is somewhat related to eager loading the gem_download association in https://github.com/rubygems/rubygems.org/blob/master/app/controllers/profiles_controller.rb#L12.

So if on master I remove the gem_download eager load and stub out the rubygem.downloads call from the view (so I don't hit the ActiveRecord::StrictLoadingViolationError exception) I see the correct invisible_captcha 2.2.0 version (this is using the most_recent_version call). There's something happening with the scope there but I'm having trouble piecing it together πŸ€”

diff --git a/app/controllers/profiles_controller.rb b/app/controllers/profiles_controller.rb
index 82a3c1a2d..2680413d4 100644
--- a/app/controllers/profiles_controller.rb
+++ b/app/controllers/profiles_controller.rb
@@ -9,7 +9,7 @@ class ProfilesController < ApplicationController

   def show
     @user = User.find_by_slug!(params[:id])
-    @rubygems = @user.rubygems_downloaded.includes(%i[most_recent_version gem_download]).strict_loading
+    @rubygems = @user.rubygems_downloaded.includes(%i[most_recent_version]).strict_loading
   end

   def me
diff --git a/app/views/profiles/_rubygem.html.erb b/app/views/profiles/_rubygem.html.erb
index c8fa121d2..df50829aa 100644
--- a/app/views/profiles/_rubygem.html.erb
+++ b/app/views/profiles/_rubygem.html.erb
@@ -7,7 +7,7 @@
       </a>
     </span>
     <p class="gems__gem__downloads__count">
-      <%= number_with_delimiter(rubygem.downloads) %>
+      <%#= number_with_delimiter(rubygem.downloads) %>
       <span class="gems__gem__downloads__heading">Downloads</span>
     </p>
     <% if local_assigns[:owners] %>

So this is what the page looks like before the PR updates

Screenshot 2024-04-21 at 14 05 05 Screenshot 2024-04-21 at 14 05 45

This is what the page looks like after the PR updates βœ…

Screenshot 2024-04-21 at 14 06 35

Copy link

codecov bot commented Apr 19, 2024

Codecov Report

All modified and coverable lines are covered by tests βœ…

Project coverage is 97.09%. Comparing base (810e853) to head (c465854).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4627   +/-   ##
=======================================
  Coverage   97.09%   97.09%           
=======================================
  Files         392      392           
  Lines        8270     8270           
=======================================
  Hits         8030     8030           
  Misses        240      240           

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

@dancristianb dancristianb marked this pull request as ready for review April 21, 2024 11:43
@markets
Copy link

markets commented Apr 22, 2024

Thanks @dancristianb πŸ‘πŸΌ πŸ‘πŸΌ Nice introspection and explanation!

@segiddins @martinemde Do you think we can merge this one πŸ™πŸΌ? Current situation is really weird.

Copy link
Member

@martinemde martinemde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to figure this out and fix it.

@martinemde martinemde merged commit dbf482e into rubygems:master Apr 23, 2024
17 checks passed
@martinemde
Copy link
Member

Deployed. Thanks again!

@markets
Copy link

markets commented Apr 23, 2024

Confirmed βœ… It works fine now in production πŸ‘Œ

Thank you so much @martinemde!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display last released version
4 participants