-
Notifications
You must be signed in to change notification settings - Fork 43
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
Fix T&C page when no apps are present #264
Conversation
|
@apps = Rails.cache.fetch(['pages/terms/app-list', ts]) do | ||
MnoEnterprise::App.order_by("name.ac").reject{|i| i.terms_url.blank?} | ||
|
||
ts = MnoEnterprise::App.order_by("updated_at.desc").first |
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 would do it this way.
ts = MnoEnterprise::App.order_by("updated_at.desc").first.try(:updated_at)
@apps = if ts
Rails.cache.fetch(['pages/terms/app-list', ts]) do
MnoEnterprise::App.order_by("name.ac").reject{|i| i.terms_url.blank?}
end
else
[]
end
what do you think?
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.
@ouranos The issue here is when there are no apps. In that case an empty array is returned. That would result in try being called on nil which will throw an error. What if we did it this way?
ts = MnoEnterprise::App.order_by("updated_at.desc").first&.updated_at @apps = if ts Rails.cache.fetch(['pages/terms/app-list', ts]) do MnoEnterprise::App.order_by("name.ac").reject{|i| i.terms_url.blank?} end else [] end
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.
It's supposed to work:
2.3.3 :001 > [].first.try(:updated_at)
=> nil
http://api.rubyonrails.org/classes/Object.html#method-i-try
The safe navigation operator is a good idea but only works with ruby 2.3+ so we'd break the compatibility with older versions (as you can see with the failing specs)
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.
Ah ok I replaced it with the try method. I was trying to run that line of code in my terminal but was using irb/pry instead of rails console. Didn't realize the method was coming from rails. 👍
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.
LGTM
Could you squash the commits and also rebase/open the PR on 3.2
git rebase -i --onto 3.2 4ca9a07^
44a4727
to
f1be5f1
Compare
[MNOE-474] Fix review UI
@ouranos There were errors when trying to go to the terms of use page. After looking at the logs it seems the error happens whenever a marketplace does not have apps yet. I'm not sure if this is actually an issue though as a dashboard wouldn't really be functional without any applications anyway. Please review.