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.

Sign up for GitHub

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

Jump to bottom

API-1: prepararations #570

Merged
merged 1 commit into from
Oct 13, 2018
Merged

API-1: prepararations #570

merged 1 commit into from
Oct 13, 2018

Conversation

wvengen
Copy link
Member

@wvengen wvengen commented Oct 13, 2018 •
edited
Loading

Part one of #429 in chunks:

  • Doorkeeper OAuth2 provider for API auth
  • A Doorkeeper app to manage API keys (Apps button from config screen)
  • An API base controller with authentication and error handling
  • Split off code common to application and API controllers to a number of concerns
  • CORS

@wvengen wvengen added the api label Oct 13, 2018
@wvengen wvengen added this to the API v1 milestone Oct 13, 2018
@wvengen wvengen requested a review from paroga October 13, 2018 14:01
@wvengen wvengen changed the title Prepare for API v1 API-1: prepararations Oct 13, 2018
@wvengen wvengen mentioned this pull request Oct 13, 2018
Merged
paroga
paroga reviewed Oct 13, 2018
View reviewed changes
app/controllers/api/v1/base_controller.rb Outdated Show resolved Hide resolved
app/controllers/concerns/auth.rb Outdated
# We have an authenticated user, now check role...
# Roles gets the user through his memberships.
hasRole = case role
when "admin" then current_user.role_admin?
Copy link
Member

Choose a reason for hiding this comment

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

can we use single quotes, if we touch the code already?

app/controllers/concerns/locale.rb Show resolved Hide resolved
app/controllers/concerns/locale.rb Outdated
end

def user_settings_language
current_user.locale if current_user
Copy link
Member

Choose a reason for hiding this comment

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

.try()?

Copy link
Member Author

Choose a reason for hiding this comment

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

:) Since we're using Ruby 2.3 anyway, why not switch to current_user&.locale?

config/application.rb Outdated
allow do
origins '*'
# this restricts Foodsoft scopes to certain characters - let's discuss it when it becomes an actual problem
resource %r{\A/[-a-zA-Z0-9_]+/api/v1/}, :headers => :any, :methods => :any
Copy link
Member

Choose a reason for hiding this comment

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

use newer syntax headers: :any?

config/application.rb Outdated
@@ -62,6 +62,14 @@ class Application < Rails::Application
# Load legacy scripts from vendor
config.assets.precompile += [ 'vendor/assets/javascripts/*.js' ]

# CORS for API
config.middleware.insert_before 0, "Rack::Cors" do
Copy link
Member

Choose a reason for hiding this comment

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

single quotes?

app/controllers/concerns/auth.rb
@@ -0,0 +1,148 @@
# Controller concern for authentication methods
Copy link
Member

Choose a reason for hiding this comment

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

I know it's just copy & past code, but it would be nice to do some cleanup if we touch it already - maybe in its own commit by just introducing the concerns

app/controllers/api/v1/base_controller.rb

def not_found_handler(e)
# remove where-clauses from error message (not suitable for end-users)
msg = e.message.try {|m| m.sub(/\s*\[.*?\]\s*$/, '')} || 'Not found'
Copy link
Member

Choose a reason for hiding this comment

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

is there no cleaner way than this ugly string manipulation?

Copy link
Member

Choose a reason for hiding this comment

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

Are there no HTTP status code strings definitions in rails? If not, can we put the hardcoded strings like Not found at least on top of the file as const defines?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't remember that there was a cleaner way, but I'll look into it again. I think it does make sense to include some sort of reason if possible.

I searched the rails code, but didn't find strings. My guess would be that symbols like :not_found are converted into readable strings.

app/controllers/concerns/locale.rb Show resolved Hide resolved
config/locales/en.yml Show resolved Hide resolved
@wvengen wvengen force-pushed the feature/api-1-preparations branch from 5b7096b to e432a53 Compare October 13, 2018 15:28
paroga
paroga approved these changes Oct 13, 2018
View reviewed changes
Copy link
Member

@paroga paroga left a comment

Choose a reason for hiding this comment

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

splitten the concerns is a separate commit would have been nice, but that's no real point. LGTM

config/initializers/doorkeeper.rb Outdated
skip_authorization { true } # right now only admins can add apps, so this is ok

# WWW-Authenticate Realm (default "Doorkeeper").
realm "Foodsoft"
Copy link
Member

Choose a reason for hiding this comment

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

would have been a candidate for single quotes 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

I missed that one! Changed it.
p.s. sorry for spamming you with attribution in the commit and force-pushing it several times ;)

Copy link
Member

Choose a reason for hiding this comment

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

sorry for spamming you

since i spam you too often, i'm not in the position of complaining 😃

@wvengen wvengen force-pushed the feature/api-1-preparations branch 2 times, most recently from 92a8223 to 3887041 Compare October 13, 2018 15:33
@wvengen
Copy link
Member Author

wvengen commented Oct 13, 2018

Thanks for your quick and useful comments! I'll merge it (maybe after the bundle update PR to get the Ruby 2.3 requirement clear, if you're ok (mysql2 had a minor update)).

@paroga
Copy link
Member

paroga commented Oct 13, 2018

sure! i really like to see it land finally 🎉

@wvengen wvengen force-pushed the feature/api-1-preparations branch from 3887041 to faaff3b Compare October 13, 2018 18:06
@wvengen wvengen merged commit fd96b6c into foodcoops:master Oct 13, 2018
@wvengen wvengen deleted the feature/api-1-preparations branch October 13, 2018 18:16
johonas pushed a commit to johonas/foodsoft that referenced this pull request Dec 17, 2018
Merge https://github.com/foodcoops/foodsoft
5b5caf5 
* https://github.com/foodcoops/foodsoft: (33 commits)
Fix financial_links table in schema.rb
API v1 navigation endpoint
API v1 config endpoint
API v1 user endpoint
Prepare for API v1 (PR foodcoops#570)
Bundle update (CVE-2018-3760, requires Ruby 2.3, PR foodcoops#561)
fix auto_close_and_send_min_quantity
Fix internal server error at invoices
Add import route for bank_transactions
Add short name to FinancialTransactionType
Add edit functionality for financial links
Add model and views for bank accounts
improve usability when ordering (PR foodcoops#552)
Add format helper for currency
Refactor FoodsoftFile to reuse the code later
Remove deprecated Gemnasium badge
Fix balancing with financial_transaction_types
Make the site_map the default view when showing all wiki pages
Add missing brackets to the Message-ID of emails
Add missing development dependency to docs
...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paroga paroga paroga approved these changes

Assignees
No one assigned
Labels
api
Projects
None yet
Milestone
API v1
Development

Successfully merging this pull request may close these issues.
2 participants
@wvengen @paroga