Hide missing title errors from user

[zachmargolis]

Gentler approach to #9447, so hopefully we do not have to rush patches like #9493 or #9521 because of user-facing 500s

[mitchellhenke]

I think I'd rather have the option to still raise (e.g. dev and test). It probably doesn't make sense to do something as complicated as the following in the template though (maybe a ViewComponent?). I think avoiding the raise/rescue and calling NewRelic directly would be preferable if possible too.

Suggested change

	title = begin
	yield(:title).presence || (raise 'Missing title (error hidden from user)')
	rescue => err
	NewRelic::Agent.notice_error(err)
	''
	end
	title = yield(:title).presence
	if title.nil? && IdentityConfig.store.raise_on_missing_title
	raise 'Missing title (error hidden from user)')
	else
	NewRelic::Agent.notice_error(err)
	end

[zachmargolis]

I wanted the raise to make sure it gets the backtrace. I'll look into the config change

irb(main):001> x = RuntimeError.new('aaa')
=> #<RuntimeError: aaa>
irb(main):002* begin
irb(main):003*   raise 'bbb'
irb(main):004* rescue => e
irb(main):005> end
=> nil
irb(main):006> x.backtrace
=> nil
irb(main):007> e.backtrace
=> 
["(irb):3:in `<top (required)>'",

[zachmargolis]

PTAL at ba43173

[aduth]

Maybe a little too "magical" / "clever", but maybe we could adapt the title application helper to contain all the logic here if it's called without an argument?

[zachmargolis]

doing that would detect if we explicitly set a nil title for a page, but it wouldn't detect the absence of a title for the page right?

[aduth]

I don't know if I'm following the concern. This is what I had in mind:

module ApplicationHelper
  class MissingTitleError < StandardError; end

  def title(title = nil)
    if title.present?
      content_for(:title) { title }
    else
      content_for(:title).presence || (raise MissingTitleError)
    end
  rescue MissingTitleError => error
    if IdentityConfig.store.raise_on_missing_title
      raise error
    else
      NewRelic::Agent.notice_error(error)
      ''
    end
  end

  # ...
end

[zachmargolis]

ahhhh yes now I see, I will make that change

[zachmargolis]

The part that confused me about your suggestion was the combined setter + getter aspect, so I split those out and made a new method in 4a2db79, PTAL!

[aduth]

Yeah, was kinda wondering if it'd be "better" or even possible to do def title=(title) to separate the methods while sharing the same name, since conceptually they overlap. But even if it did work, it'd touch a lot of files, so not necessarily something I'd propose to take on here.

[zachmargolis]

We'd have to make the templates call self.title = because otherwise Ruby will parse it as a local variable assignment, but yes that would cause a lot of diff noise that I'd like to avoid for now

[aduth]

I was interested 😄 #9529