Features controller refactor #2480

seafr · 2018-03-12T15:07:12Z

No issue opened.

tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
code is in uniquely-named feature branch and has no merge conflicts
PR is descriptively titled

Thanks!

… route for redirect

…med route for redirect

PublicLabBot · 2018-03-12T15:15:24Z

	2 Messages
📖	@seafr Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.
📖	This pull request doesn’t link to a issue number. Please refer to the issue it fixes (if any) in the body of your PR, in the format: `Fixes #123`.

Generated by 🚫 Danger

seafr · 2018-03-12T15:25:01Z

@jywarren this is related to the refactor in #2448

jywarren · 2018-03-12T19:31:02Z

app/controllers/features_controller.rb

      flash[:warning] = 'Only admins may edit features.'
-      redirect_to '/features'
+      redirect_to features_url


Is this auto-generated from the routes, then? Thanks!

Yes, features_url and features_path. The first is recommended for redirection as it contains the full URL.

I want to add that I rarely come across named routes in the codebase. And I don't see Rails' link_to method. Is there a specific reason for this?

In general, I've felt that it is more readable for people who just do HTML (which is a lot of people) to just use <a> tags, and the _url Rails helpers are a little mysterious to people. I can see the advantage to having managed routes, but I think there's something good about having a literal link where you can see the URL, in a standard HTML format.

But I'm open to discussion on it! What do you think?

I see your point, esp. regarding templates. However, controllers are just Ruby so named routes would probably suit them better. The idea is to enable any future change of URLs without breaking things, like what would happen in #2484 (/blog -> /wiki/stories). But I don't want to be disruptive here. Maybe you think it's better to keep things as they are (for now at least). Or maybe we should get input from other contributors too. Let me know your thoughts and I'll be glad to revert relevant changes back if needed.

How about in order to move forward, we stick with static string routes for now, and then as you said

get input from other contributors too

That sounds great. I do think we have trouble with route maintenance -- #1841 was trying to think through that one -- and very occasionally routes break since they are not thoroughly tested :-(

Thanks for thinking this through! I agree re: controllers being in ruby, but the named routes are just so mysterious to newcomers! If we could smooth that learning curve a bit I'd be more comfortable using them.

Yes, the routes file needs a bit of tidying up.
OK I'll use static strings for now. I agree that named routes can be a mystery for newcomers. Very relevant to our newcomer-friendly policy!
Last, this is not a promise, but I'll try to look into the routes issue and see if I can help. Anything you think you should mention?

Could you tell me why the current time is (sometimes) appended to the redirect URL? Like in redirect_to '/features?_=' + Time.now.to_i.to_s

seafr · 2018-03-12T20:49:06Z

@jywarren, please note that the first 2 commits have already been merged (#2448).
In fact, only changes in app/controllers/features_controller.rb are relevant.

jywarren · 2018-03-14T22:37:45Z

Looks great, is this ready to go? :-)

seafr · 2018-03-15T15:34:49Z

Yes, I guess so. But there has been no formal review by anybody, hence the review tag.

grvsachdeva · 2018-03-15T17:41:59Z

app/controllers/features_controller.rb

@@ -12,14 +12,14 @@ def embed
  end

  def new
-    if current_user.role != 'admin'
+    unless current_user.admin?


hi @seafr , in all other places, you have used if !current_user.admin?. how about following the same syntax here too, only if you want to?

I used if ! elsewhere due to the presence of the else clause. I find it hard to understand else after unless, else I'd use unless everywhere.

P.S. I have a feeling there is an overuse of else in the above text though.

yeah unless/else trips me up too. Interestingly -- I think double negatives also vary in use across languages!

grvsachdeva · 2018-03-15T17:42:49Z

app/models/user.rb

+    # use instead of "user.role == 'admin' || user.role == 'moderator'"
+    admin? || moderator?
+  end
+


Nice work !!!

grvsachdeva · 2018-03-15T17:43:24Z

test/unit/user_test.rb

+    assert_not basic_user.admin?
+    assert_not basic_user.moderator?
+    assert_not basic_user.can_moderate?
+  end


test seems fine to me

Thank you for review. 👍

grvsachdeva

Awesome work !!!

jywarren · 2018-03-16T09:38:44Z

Thanks @Gauravano for such a thorough review! Thank both of you for your thorough work! I also notice both of you helping others out a lot and with great attitude 😄

Merging away! ✈️

grvsachdeva · 2018-03-16T10:21:15Z

Thanks, Jeff !!! Just performing our part being a member of the PublicLab community 😊

seafr · 2018-03-16T12:44:51Z

@jywarren thank you for the kind words. It's a pleasure to be part of the PL community. We are just trying to give back the welcome we received as newcomers. Ideal place to get into OSS.

* Add methods to User model for user roles * Add a comment * Edit 'new' to use User#admin? to verify user's role, and to use named route for redirect * Modify 'edit' to use User#admin? to verify user's role, and to use named route for redirect * Modify 'create' & 'update' to use User#admin? to verify user's role * Revert back to literal routes from named routes

seafr added 5 commits March 8, 2018 20:11

Add methods to User model for user roles

8506ed4

Add a comment

b58c93b

Edit 'new' to use User#admin? to verify user's role, and to use named…

5b95ca6

… route for redirect

Modify 'edit' to use User#admin? to verify user's role, and to use na…

18c001a

…med route for redirect

Modify 'create' & 'update' to use User#admin? to verify user's role

476c533

seafr added the review-me label Mar 12, 2018

jywarren reviewed Mar 12, 2018

View reviewed changes

Revert back to literal routes from named routes

9cab0d9

grvsachdeva reviewed Mar 15, 2018

View reviewed changes

seafr added ready and removed review-me labels Mar 15, 2018

grvsachdeva approved these changes Mar 15, 2018

View reviewed changes

jywarren merged commit 4517567 into publiclab:master Mar 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Features controller refactor #2480

Features controller refactor #2480

seafr commented Mar 12, 2018

PublicLabBot commented Mar 12, 2018 •

edited

Loading

seafr commented Mar 12, 2018

jywarren Mar 12, 2018

seafr Mar 12, 2018 •

edited

Loading

seafr Mar 13, 2018

jywarren Mar 14, 2018

seafr Mar 14, 2018

jywarren Mar 14, 2018

jywarren Mar 14, 2018

seafr Mar 14, 2018

seafr Mar 21, 2018 •

edited

Loading

seafr commented Mar 12, 2018 •

edited

Loading

jywarren commented Mar 14, 2018

seafr commented Mar 15, 2018

grvsachdeva Mar 15, 2018

seafr Mar 15, 2018

jywarren Mar 16, 2018

grvsachdeva Mar 15, 2018

seafr Mar 15, 2018

grvsachdeva Mar 15, 2018

seafr Mar 15, 2018

grvsachdeva left a comment

jywarren commented Mar 16, 2018

grvsachdeva commented Mar 16, 2018 •

edited

Loading

seafr commented Mar 16, 2018

Features controller refactor #2480

Features controller refactor #2480

Conversation

seafr commented Mar 12, 2018

PublicLabBot commented Mar 12, 2018 • edited Loading

seafr commented Mar 12, 2018

Choose a reason for hiding this comment

seafr Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seafr Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

seafr commented Mar 12, 2018 • edited Loading

jywarren commented Mar 14, 2018

seafr commented Mar 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grvsachdeva left a comment

Choose a reason for hiding this comment

jywarren commented Mar 16, 2018

grvsachdeva commented Mar 16, 2018 • edited Loading

seafr commented Mar 16, 2018

PublicLabBot commented Mar 12, 2018 •

edited

Loading

seafr Mar 12, 2018 •

edited

Loading

seafr Mar 21, 2018 •

edited

Loading

seafr commented Mar 12, 2018 •

edited

Loading

grvsachdeva commented Mar 16, 2018 •

edited

Loading