Follow-up from "Put back broadcast messages to sign-in page for self-hosted"
The following discussions from !128234 (merged) should be addressed:
-
@syarynovskyi started a discussion: (+3 comments) @jmontal Could you pls review backend ?
PS Considering MR has only small
*.haml
changes, I think we can skip frontend -
@jmontal started a discussion: suggestion (non-blocking): WDYT about wrapping this check in a context similar to the SaaS context, for example:
context 'when self-hosted' do it 'does not render the broadcast layout' do render expect(rendered).not_to render_template('layouts/_broadcast') end end
-
@jmontal started a discussion: suggestion (non-blocking): WDYT about moving
render
to the before block so each context is purely it's expect statement? -
@jmontal started a discussion: suggestion (non-blocking): Similar to the other spec, WDYT about moving
render
to the before block so each context is purely it's expect statement? -
@jessieay started a discussion: Question (non-blocking): @syarynovskyi What would you think about creating an application setting for this?
This is the recommended approach instead of using
Gitlab.com?
checks in the code. And one day, we hope to ban use of these checks. So adding this is adding one more place where it will need to be removed.I see on the related issue that there was the intention to "fast track" this fix, and adding this check as-is certainly is the fastest option. But I think it warrants a follow-up issue to put this logic behind a setting instead so that an instance can opt into having broadcast messages on sign in and sign up pages or not.