Skip to content
This repository has been archived by the owner on Jul 24, 2020. It is now read-only.

View catalogue / homepage without login #175

Closed
6 tasks done
dgoerger opened this issue Jul 11, 2012 · 22 comments · Fixed by #1070
Closed
6 tasks done

View catalogue / homepage without login #175

dgoerger opened this issue Jul 11, 2012 · 22 comments · Fixed by #1070

Comments

@dgoerger
Copy link
Contributor

We should have a homepage with at least:

  • visible to non-logged in

We might also want it to have:

  • general info about the app / project
  • feed of recently added models, or featured models

general dashboard layout.. :-D


#2014/12/18

  • Add integration tests for guest functionality
    • Functionality of basic routes
    • Authorization of invalid routes (e.g. /reservations/new, etc)
    • Use of catalog features
    • App config setting (see next)
  • Add app config setting to disable guest user functionality
@dgoerger dgoerger changed the title Legit home / splash page? View catalogue / homepage without login Jun 13, 2014
@dgoerger
Copy link
Contributor Author

Was: "Legit home / splash page?"

We could perhaps allow non-logged in users to view the catalogue by implementing a special "dummy user", which would redirect to login if they tried to add anything to the cart etc, if we want to do this. Most online catalogues allow browsing before setting up an account.

@mnquintana
Copy link
Contributor

I like that approach - it would let us keep the catalog DRY.

@mnquintana mnquintana added this to the 4.0.0 milestone Jul 1, 2014
@dgoerger
Copy link
Contributor Author

note:

  • search engines won't be able to find the catalogue until they can reach it without CAS login
    • we should allow admins to set the option, in case they want a private instance

@orenyk
Copy link
Contributor

orenyk commented Sep 21, 2014

Some notes after ~1 hr of work:

  • we can selectively disable / replace the default rubycas before_filter with a gateway filter for certain controller actions; this means that CAS login is not required but the information will be used if the user is logged in to CAS.
  • we need to add a working login link for the navbar
  • we would like to use a guest user to utilize CanCanCan; we're thinking putting in rake app:setup and just creating a dummy user with role and view_mode set to guest. We would probably then have to modify the current_user method to default to the guest user if there has been no CAS login.
  • you can't use the can? method when determining whether or not to run a before_filter
  • you also can't use the session when determining whether or not to run a before_filter

@orenyk
Copy link
Contributor

orenyk commented Sep 21, 2014

Ok, some more notes:

  • we have defined a guest_user helper method in the ApplicationController to find a user in the DB w/ role and view_mode set to guest or create one if none exist.
  • we have modified the current_user method to use the guest user if not logged in to CAS or pull from the database otherwise

TO DO:

  1. make sure we specify the abilities of guest users
  2. remove the cart from the catalog / equipment model show pages for guests
  3. remove the add to cart buttons for guests

@orenyk
Copy link
Contributor

orenyk commented Sep 28, 2014

Turns out we forgot to add guest to the list of allowed roles / view modes. This has since been corrected.

@orenyk
Copy link
Contributor

orenyk commented Sep 28, 2014

On second thought, we need to be moving away from CAS specifically (see #2) so any changes we make right now will be specific to our current authentication setup and will have to be changed after we implement something like Devise. We're going to start working on #2 and come back to this once that is finished.

@orenyk orenyk mentioned this issue Sep 28, 2014
31 tasks
@orenyk
Copy link
Contributor

orenyk commented Sep 28, 2014

If we switch to Devise, they include guest user functionality!

@caseywatts
Copy link
Collaborator

They could probably even put things in their cart before creating an account, that's a bonus feature! :D

@orenyk
Copy link
Contributor

orenyk commented Sep 28, 2014

Yup that's what we were hoping for. We'll get back to this after #2 is finished.

@orenyk orenyk mentioned this issue Oct 13, 2014
13 tasks
@orenyk
Copy link
Contributor

orenyk commented Oct 14, 2014

I've taken the first step of disabling authentication for the catalog controller in the 2_authentication_II branch to deal with an issue I was having there. We still need to fix the views / behavior of the app to be sane when no one is logged in, so there's still a bunch to do :-). We should also implement a guest user using Devise as linked above.

@orenyk
Copy link
Contributor

orenyk commented Oct 15, 2014

Ok, most of this has been completed in #2. We should still review the behavior of the app for guests and also write some tests. We should also allow admins to configure if guest users are allowed.

@orenyk orenyk modified the milestones: 4.1.0, 4.0.1, 4.1.1 Oct 27, 2014
@orenyk orenyk assigned orenyk and unassigned aaron-effron Dec 19, 2014
@orenyk
Copy link
Contributor

orenyk commented Dec 19, 2014

Ok, so I'm pretty sure that the guest functionality works as expected. In particular, guests can:

  1. Browse the catalog
  2. Add / remove items from the cart
  3. Change the cart dates
  4. View equipment model show views
  5. View the categories/:id/equipment_models view

The navbar has a 'Sign In' link as does the cart. At this point, I think I'll write some integration tests to verify that our authorization is working as expected (e.g. can't visit /reservations/new and a few other views) and add an app config to disable guest user functionality.

@orenyk
Copy link
Contributor

orenyk commented Dec 19, 2014

Ok, these tests are mostly written, albeit dependent on a CSS id that is introduced in #1068 (hence they will fail until that is merged into master and subsequently merged into this branch). I'm not sure how to test changing the cart dates since we're using JS to submit that form, but once that's done we can implement the app config setting, test it, and be done :-).

@orenyk
Copy link
Contributor

orenyk commented Dec 19, 2014

Ok, so this is resolved except I'm having trouble testing the setting. For some reason, I can use a before :each block to modify the setting in the database and it's changing the setting in the test context but that's not translating to the "server". For example, puts AppConfig.first.enable_guests returns the correct thing but Rails follows whichever setting gets applied first (depending on the describe block that goes first). Any thoughts?

@orenyk
Copy link
Contributor

orenyk commented Dec 19, 2014

The functionality has been tested manually and the setting does what's expected (forces authentication on all requests if guest functionality is disabled). There is a potential issue with an error flash showing up after logging in w/ CAS under those circumstances, I can try to deal with it once we resolve the testing issues.

@orenyk
Copy link
Contributor

orenyk commented Dec 25, 2014

This is incredibly frustrating; I tried splitting the tests into two describe blocks and the issue is still occurring. I'm going to try getting rid of the shared examples (unfortunately) and see if that helps.

@orenyk
Copy link
Contributor

orenyk commented Dec 25, 2014

OOOOOOOOOOOOOOOOOOOK, I (edit: kinda) figured it out. Rails is caching something between requests in test so despite the fact that we're both writing new objects between each test and sanitizing the database. If I set config.reload_classes_only_on_change = false all the tests pass, although they're super slow. That said, I still can't figure out why this is happening, despite trying to dig through the test logs.

It appears as though this doesn't affect CanCan, since when the disabled context goes second, all of the failed tests are either visits to the homepage or redirects to the homepage from CanCan when trying to access a forbidden action (as set by our AppConfig.first.enable_guests parameter). However, for some reason the logic check in the skip_before_filter call in the CatalogController isn't happening, or it's returning true for AppConfig.first.enable_guests even when it's set to false.

This is becoming a huge pain; the code works but we're somehow in a weird edge case where RSpec and Rails aren't playing nicely together. I'll give this a little more time and then just try to get this reviewed / merge as is (since it's working manually).

@orenyk
Copy link
Contributor

orenyk commented Dec 25, 2014

Unfortunately, changing that setting breaks a whole bunch of tests (801 out of 1409, to be precise) :-( - I think it has something to do w/ threading. The error message is A copy of [User / whatever Model we're creating] has been removed from the module tree but is still active! and it gets thrown when factory_girl tries to create stuff. Blergh.

@orenyk
Copy link
Contributor

orenyk commented Dec 29, 2014

OMG I GOT IT!

@orenyk
Copy link
Contributor

orenyk commented Dec 29, 2014

Sorry, that had to be on it's own comment. Wow. Ok, so I was using conditional before filters incorrectly and this resulted in class caching preventing later tests from checking the conditional. I largely figured it out from this SO thread, particularly this answer (when I was trying to implement a conditional skip_before_filter in ApplicationController).

Basically, rather than use Ruby conditionals to evaluate whether or not to run (or skip) a before_filter, I should have defined a controller method to evaluate whether or not to run/skip said before_filter. For example,

skip_before_filter :authenticate_user! if (AppConfig.first && AppConfig.first.enable_guests)

becomes

skip_before_filter :authenticate_user!, unless: guests_disabled?

def guests_disabled?
  AppConfig.first && !AppConfig.first.enable_guests
end

Some similar hackery to deal with the generalized actions in ApplicationController and all tests are now passing :-D.

@orenyk
Copy link
Contributor

orenyk commented Dec 29, 2014

Also, I learned about using the Rails logger which was super useful, see here.

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

Successfully merging a pull request may close this issue.

6 participants