FIX: allows the selection of the default landing tab (PR #481)

Meta: Where does one select the default landing tab?

Screenshot_17_03_13_00_01

  • associates the / route to the first item in the top_menu site setting (1) in order to mark the right item in the menu as active when visiting the homepage
  • prevents duplicate routes when the categories is the default landing page
  • removed duplicate static routes declaration

(1) my solution to retrieve the top_menu data from the site settings feels a bit hacky-ish. Unfortunately, I can’t use the default PreloadStore API as it’s a read-only-once structure and would delete its content.

GitHub

1 Like

You’ve signed the CLA, ZogStriP. Thank you! This pull request is ready for review.

Sorry @SamSaffron I overwrote my previous commit. I feel that this one is better (there was actually no need to modify the default top_menu site setting values).

The only remaining question is this line.

This looks good to me. What’s the issue with the split?

@eviltrout The issue is more with PreloadStore.data.siteSettings.top_menu which is getting internal data from the PreloadStore without using its API.

@ZogStriP oh I see. Yeah, that is a difficult one. Is the PreloadStore API not available when the routes are created? I believe routes are created following everything else.

The issue is that if I use the API (ie. PreloadStore.getStatic('siteSettings')) it will delete the data and this line won’t work anymore.

I think we should amend the API for preloadstore so its explicit.

It should be

PreloadStore.getCached('siteSettings')
PreloadStore.deleteCached('siteSettings')

I don’t like this getStatic api its deleting stuff, the name should reflect it.

Optionally we could just allow .get to return direct data if called without a finder, but that is ugly cause you are returning 2 different types of things (promise and raw)

I agree that a name of get should probably not delete, but it is very convenient for most API calls to do that. How about this:

We add separate get and delete calls so the router can just use get AND we rename the old get to be getAndDelete so it’s obvious what you’re doing?

@eviltrout That’s what I had in mind. This is on my TODO list for today :wink:

:+1:

awesome, can you update this to the latest api so we can merge?

@SamSaffron sure, I was waiting for you to merge #505 :wink:

sure … np … its merged now

Ok, so this turns out to be tougher than expected. But I think I got it sorted out.

Here’s what I’ve done:

  • cleaned up nav_item.js
  • removed what I found to be useless code in list_controller (might be wrong here)
  • the index method of the list_controller is no longer an hard-coded alias to the popular filter (need to take into account the top_menu site setting and properly handle anonymous vs. logged-in users)
  • changed the routes for the popular filter to list#popular instead of the hard-coded list#index

I’m having one issue for anonymous users though:

Anonymous users can only see two filters: popular and categories (all the others requires an account). When the administrator choose to display the categories filter before the popular one it will become the home page for anonymous users.

But, categories are not just another filter. They’ve got their own controller. I’ve used a redirect_to but this causes a 302 which is obviously not performant. Not sure what’s the best way to deal with this case…

Feedbacks are welcome :wink:

I have been thinking about this.

I really dislike doing router work in the controller. What should happen is this.

  • on bootup we check the setting and set up routes correctly based on it - so index either goes to /categories or /popular or whatever

this gets us almost there, except that we have an issue now that certain settings require an app restart, we have 2 options:

  1. dick with the router based on setting changes - I would like to defer this work cause its risky even though I think we may get to it with plugins
  2. have a special app flag that is triggered if you dick with certain settings and is communicated to all the processes, saying a restart is required for some settings to take effect.

eg:

site_setting(:bla_bla_bla, “default”, requires_restart: true)


I think we should get the site_setting change in first in a separate PR then we can dick with the routing based on site settings at boot time.

thoughts @eviltrout ?

@SamSaffron This is a better solution indeed. I’ll take care of the site_setting change in another PR.

However, this does not solve the anonymous vs logged-in user case but I think using a route constraint should do the trick. What do you guys think?

one huge complication with the routing approach is multi site support, we need to have a think about it, its as if we want to switch out parts of the router depending on the site you are on.

The same process that is serving discuss.emberjs.com is serving dev.discourse.org , one of the sites could have the override and another not.

Nonetheless I think this is the right direction to take.

On Fri, Mar 22, 2013 at 9:29 PM, Régis Hanol notifications@github.comwrote:

@SamSaffron https://github.com/SamSaffron This is a better solution indeed. I’ll take care of the site_setting change in another PR.

However, this does not solve the anonymous vs logged-in user case but I think using a route constraint should do the trick. What you guys think?

— Reply to this email directly or view it on GitHubhttps://github.com/discourse/discourse/pull/481#issuecomment-15289975 .

I am definitely on board with the requires_restart option. One thing that’s important though is to allow the user to change multiple settings before restarting. Perhaps once they change one, it adds a button at the top saying “You need to restart discourse for settings to take effect” that stays there until they click or restart.

@ZogStriP we can probably use constraints to account for the subdomains see:

http://edgeguides.rubyonrails.org/routing.html#request-based-constraints

something along:

 root to: 'list#popular',
        constraints: HasPopularHomePageConstraint.new

or even generate this in a loop or something …