[ZL-887] Use Procore Ruby SDK by njbbaer · Pull Request #14 · procore/Procore-Sample-ROR

njbbaer · 2021-02-26T07:31:58Z

This PR rewrites and simplifies the sample app by using the Procore Ruby SDK.

…ap-sass

njbbaer · 2021-03-11T08:16:15Z

I'm finally ready to open this up for review 🎉

njbbaer · 2021-03-11T08:23:26Z

I just need to make sure the latest version of the Ruby SDK is being used once procore-oss/ruby-sdk#39 has been released.

claudioprocore

Hello there!

The title of this PR is "[WIP] Use Procore Ruby SDK" and the description says that this "is currently a work in progress." so I'm not sure if this was ready for review.

Meanwhile, I left some inline comments based on the existing commits.

claudioprocore · 2021-03-11T15:46:04Z

-    # # Store the parsed response in an instance variable
-    # @me = JSON.parse(get_me)
+    # Store the parsed response in an instance variable
+    @me = request.body


What does this body look like?

From docs https://developers.procore.com/reference/rest/v1/me

{ "id": 160586, "login": "exampleuser@website.com", "name": "Carl Contractor" }

claudioprocore · 2021-03-11T15:48:37Z

-          <% end %>
-        </ul>
+      <%= render "layouts/nav_bar" %>
+      <%= render "layouts/flash_message" %>


Rails normally refers to layouts as something that "wraps around" the content to display.

For instance, this file is a layout because it renders the head and then <%= yield %> to the content.

Instead, portions of HTML rendered inside another view are called partials.

So I would suggest renaming these files to partials.

They aren't layouts, but they're also only used by application.html.erb which is a layout. So I figure it's logical to put them in the same folder, like you might put partials next to /users if they were only used on that page. Partials are prefixed by an _ to distinguish them.

I'll also point out Dev Portal puts some partials in /layouts and doesn't have a /partials directory. Doesn't make it right though 😉

claudioprocore · 2021-03-11T15:50:43Z

+    # Redirects user back to the login page
+    redirect_to login_index_path, success: "Token was successfully revoked"
+  rescue Procore::Error
+    redirect_to login_index_path, danger: "Something went wrong. Please try again."


Could this cause an infinite loop?

Let me explain: the login_index is displayed to users who are not logged in.

But in this case, if client.revoke raises a Procore::Error, then the reset_sessions line is skipped.
Therefore the user might still have a session, which would try to redirect the user out of the login_index page and possibly cause an infinite redirection.

That's a good thought, but it isn't a problem because having a session does not cause the user to be redirected out of login_index. They could log in twice, regardless of whether they still have a session.

claudioprocore · 2021-03-11T15:51:40Z

-    redirect_to users_home_path
+    redirect_to users_home_path, success: "Access token granted."
+  rescue Procore::Error
+    redirect_to login_index_path, danger: "Something went wrong. Please try again."


What is Procore::Error?

Is it something that could be solved by trying again?

It's any error from the RubySDK. If this happens something is probably misconfigured, so trying again wouldn't help. I can remove that part.

There's a list of error types here: https://github.com/procore/ruby-sdk#error-handling

I'm going to change it to print out the message, so the user has more information about what went wrong.

The message includes more information now:

claudioprocore · 2021-03-11T15:52:55Z

-  #       redirect_to login_index_path, danger: "Something went wrong. Please try again."
-  #     end
-  #   end
+  rescue_from Procore::Error do |exception|


I saw in the previous commit that Procore::Error was specifically rescued by several controlleres.

What is the case for this "generic" rescue_from?
Which actions might raise a Procore::Error but don't rescue it?

It's a generic error from the RubySDK. Anything that's making an API request with the SDK could raise it, and those should all be rescued.

claudioprocore · 2021-03-11T15:53:59Z

    </div>
-<% end %>
+<% end %>
+<% flash.clear %>


By default, Rails clears flash messages once they are displayed.

Is there anything that wasn't cleared by the code above?

I found /users/me was not clearing errors. I think this is because they only gets cleared on a redirect, and it doesn't redirect. Let me know if there is a better way to handle this.

Procore-Sample-ROR/app/controllers/users_controller.rb

Lines 12 to 14 in 129277b

rescue Procore::Error

flash[:danger] = "Something went wrong. Please try again."

end

claudioprocore · 2021-03-11T15:56:21Z

+
+  <div class="form-group">
+    <label for="version">Version:</label>
+    <input id="version" type="text" name="version" placeholder="v1.0">


Does this tag need a closing tag or a self-closing />?

Input is a void element, so it doesn't require a closing tag. Most examples don't seem to use it either: https://www.w3schools.com/tags/tag_input.asp

gilmcquillan · 2021-03-12T16:49:01Z

@njbbaer I think the README.md needs to be reviewed and updated.

davismartin

overall LGTM just one small question

The only other things I might add is

it might be nice to update the readme. To call out this uses the ruby-sdk and also might be nice to guide a user to what they need setup in procore client_id/secret/redirect_uri just to make sure that is setup correctly.
It might be nice to default the api request to a certain value so when the user auth successfully they can just hit submit and see it working

njbbaer · 2021-03-15T07:40:24Z

It might be nice to default the api request to a certain value so when the user auth successfully they can just hit submit and see it working

It would be a nice feature, but I ran into a problem where the default was overwriting the current request. I'm sure it can be fixed, but I'm just going to skip it for now so since it's low value.

njbbaer · 2021-03-15T07:42:51Z

@claudioprocore @gilmcquillan @davismartin Thanks for all your great comments. I think I've addressed them all, please respond and rereview.

njbbaer added 7 commits February 16, 2021 22:37

Ignore custom rubocop configuration

f6afab7

Add procore gem dependency

eef543e

Remove boostrap gem since it's unnecessary and conflicts with bootstr…

faddf80

…ap-sass

Add pry debugger gem

474dd05

Set up Ruby SDK and begin rewrite

4ef0b53

Refactor routes

8e49400

Implement /users/me page

210ce8b

njbbaer self-assigned this Feb 26, 2021

njbbaer mentioned this pull request Feb 26, 2021

[ZL-871] Migrate to Rest v1.0 #15

Merged

njbbaer and others added 9 commits March 10, 2021 00:17

Remove unused files

97dbf50

Show flash messages on every page & refactor erb into partials

eeeb7dd

Revoke access token with Ruby SDK

02c6992

Refresh access token with Ruby SDK

7d99c87

Refactor login controller

713a080

Merge branch 'master' into ruby-sdk

c107ca2

Handle errors for /me route

58cc557

Update custom requests to use Ruby SDK

129277b

Minor code style fixes

58ea4c7

njbbaer marked this pull request as ready for review March 11, 2021 08:14

njbbaer requested review from davismartin, gilmcquillan and nick-morrill March 11, 2021 08:14

njbbaer requested review from adamwells and claudioprocore March 11, 2021 08:24

claudioprocore reviewed Mar 11, 2021

View reviewed changes

njbbaer changed the title ~~[WIP] Use Procore Ruby SDK~~ [ZL-887] Use Procore Ruby SDK Mar 11, 2021

Use textarea element for body

5ac5579

gilmcquillan reviewed Mar 12, 2021

View reviewed changes

Comment thread app/controllers/users_controller.rb Outdated

Update procore gem

a8e1f7b

davismartin reviewed Mar 12, 2021

View reviewed changes

Comment thread app/controllers/application_controller.rb Outdated

njbbaer added 2 commits March 12, 2021 14:38

Fix typos

f25e6b4

Update readme

c62bd74

gilmcquillan reviewed Mar 12, 2021

View reviewed changes

Comment thread README.md Outdated

gilmcquillan reviewed Mar 12, 2021

View reviewed changes

Comment thread README.md Outdated

njbbaer added 2 commits March 14, 2021 23:53

Clean up readme text

f10ea8d

Display error information on SDK failure

d9d2740

claudioprocore approved these changes Mar 15, 2021

View reviewed changes

davismartin approved these changes Mar 15, 2021

View reviewed changes

gilmcquillan approved these changes Mar 15, 2021

View reviewed changes

njbbaer merged commit c5deb80 into master Mar 15, 2021

njbbaer deleted the ruby-sdk branch March 15, 2021 21:19

	rescue Procore::Error
	flash[:danger] = "Something went wrong. Please try again."
	end

Conversation

njbbaer commented Feb 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

njbbaer commented Mar 11, 2021

Uh oh!

njbbaer commented Mar 11, 2021

Uh oh!

claudioprocore left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

njbbaer Mar 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gilmcquillan commented Mar 12, 2021

Uh oh!

davismartin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

njbbaer commented Mar 15, 2021

Uh oh!

njbbaer commented Mar 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

njbbaer commented Feb 26, 2021 •

edited

Loading

njbbaer Mar 12, 2021 •

edited

Loading