Skip to content
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

Fixes #326, Fixes #593: AdminsController finishing #498

Merged
merged 33 commits into from
Nov 14, 2018

Conversation

armahillo
Copy link
Collaborator

@armahillo armahillo commented Oct 9, 2018

This PR finishes the AdminsController namespace (Started in #493)

Completed so far (in this PR):

  • BarcodeItem CRUD
  • CanonicalItems CRUD
  • Admin Dashboard
  • Users CRUD
  • Organizations CRUD

Most still need specs.

@armahillo armahillo force-pushed the 326-admins-controller branch 4 times, most recently from d46dc3d to b328cfc Compare October 25, 2018 02:48
…ganization dashboard to nav. Adds a few feature specs for that.
jcavena and others added 4 commits November 4, 2018 21:25
… into 326-admins-controller

# Conflicts:
#	app/controllers/admin/barcode_items_controller.rb
… into 326-admins-controller

# Conflicts:
#	app/controllers/admin/barcode_items_controller.rb
@armahillo armahillo changed the title WIP :donut: merge yet - AdminsController finishing WIP :donut: merge yet - Fixes #326: AdminsController finishing Nov 6, 2018
…d items with barcode items. Specs to cover it. Modifies JS to handle new method for barcodes.
@armahillo
Copy link
Collaborator Author

Fixes #593

With the newest updates. I finished the majority of the additional features required for that Issue.

There may be additional CRUD to add to Admin namespace, will need to check.

Copy link
Member

@seanmarcia seanmarcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it, great info in there that will be helpful for "at a glance info."
What do you think about also putting in recent users? Something like:
@active_users = User.where('last_sign_in_at > ?', 1.week.ago) or something like that. So we can see who is actively using the system.

@armahillo armahillo changed the title WIP :donut: merge yet - Fixes #326: AdminsController finishing Fixes #326, Fixes #593: AdminsController finishing Nov 13, 2018
@armahillo
Copy link
Collaborator Author

@seanmarcia OK I think this is ready. It incorporates teh changes @jcavena made as well. I also added a basic Dashboard in the admin area.

@seanmarcia
Copy link
Member

I just made some comments on a few things I think were mistakes from copy pasta and a suggestion. (That I'm also happy to implement.)

@armahillo
Copy link
Collaborator Author

@seanmarcia Yup -- Rspec caught all those mistakes, whoops!

Not sure about gravatar, but it gives "an icon" regardless -- if they have one registered, all the better, haha. :)

I like the idea for the additional stat!

<ul class="users-list clearfix">
<% @recent_organizations.each do |org| %>
<li>
<%= image_tag(org.logo) %>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my other suggestion of using logo_path might make this not break if they don't ahve a logo uploaded. Maybe active storage handles this gracefully if they don't.

@jcavena
Copy link
Collaborator

jcavena commented Nov 13, 2018

@seanmarcia @armahillo The "active users" won't work the way you think unless we implement :timeoutable. The db is updated when they sign in, but not while they remain signed in. After being active and signed in for 10 days, they'll fall off the list. There are probably other devise helper gems to aid in this as well.

@seanmarcia
Copy link
Member

@armahillo I just added to your PR. I put in the @active_users to the dashboard that looks for people logged in in the past week and then displays their name, email, organization, and last login time. I took JC's suggestion and added :timeoutable to the devise configurations on the user model and in the devise initializer I set the timeout time to 600 minutes.

def dashboard
@recent_organizations = Organization.where('created_at > ?', 1.week.ago)
@recent_users = User.where('created_at > ?', 1.week.ago)
@active_users = User.where('last_sign_in_at > ?', 1.week.ago)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seanmarcia This is the "previous" sign in date, not the "current" one. You want current_sign_in_at for their most recent sign in date.

600 minutes is 10 hours. You're likely going to log people out every day for this "nice to have". I'm not sure it's a good idea to do that in order to have an admin "active users" list.

`last_request_at` is updated at a minimum 10 minute interval so it doesn't update the database on each request.
@jcavena
Copy link
Collaborator

jcavena commented Nov 14, 2018

@seanmarcia @armahillo I implemented last_request_at and it updates at most every 10 minutes while a user is active. I upped timeoutable to 7 days instead of 10 hours so we're not logging out the users overnight every day.

@seanmarcia seanmarcia merged commit fa57dbd into master Nov 14, 2018
@IlinDmitry IlinDmitry deleted the 326-admins-controller branch February 8, 2019 05:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants