add resume upload #265

afogel · 2023-08-27T16:48:39Z

What's the change?

This PR enables users to upload and update their resumes. This PR also enables direct upload, such that files get stored directly on a storage service and then the attachments are created via signed_id rather than an io to the filename.

In this PR, given that the Resume lives on the user, but the resource is a Profile (and they are only related via User), I encapsulated the form using the form object pattern for validations/collecting attributes, and passed it to a service/command object to process the relevant business logic.

Finally, I reorganized the form so have a bit more logical groupings, making it easier to skim and parse. Though I don't think it's in scope here, I think we should probably redesign the profile page form.

Highlights / Surprises / Risks / Cleanup

Demo / Screenshots

https://www.veed.io/embed/0667b780-b21b-4211-be27-8e58a82503e9

Issue ticket number and link

#179

Checklist before requesting a review

Please delete items that are not relevant.

spec/commands/profiles_update_spec.rb

app/views/profiles/edit.html.erb

toyhammered

I've left some comments and thoughts. Let me know what you think and if what I said makes sense.

toyhammered · 2023-08-27T20:50:26Z

app/commands/profiles/update.rb

@@ -0,0 +1,52 @@
+module Profiles


So tbh I am not a big fan of this type of abstraction for the most part. To solve one of my problems, the call! should either return nothing or raise an error. Alternatively, you could make it call which can return true/false, but I much prefer raising custom errors specific to what went wrong.

(Another solution would be to pass in the controller in via self).

Now, given that we will be having a resume upload needed in the user_mentee_application I think tailoring to that specifically, using a service object is acceptable. That being said, what should be passed in here is most likely a user and a resume.

There might be some complications with figuring out how to properly redirect, but we can figure that out later.

When you have some time, I'd be curious to hear a bit more about the time using this type of command/service object abstraction to encapsulate domain logic ended up causing an issue for you.

I'm also unclear about why you prefer call! to return nothing or raising an error? I can understand wanting to bubble up an error, though as long as that error gets handled and reported (we probably could look into rollbar, honeybadger, or something similar), seems like gracefully recovering from it is important.

I have historically referenced the stitchfix post on anatomy of a service object as rough guidelines and specifically like returning rich objects. In this case, I chose to use symbols over a more complex object to simplify the implementation so I could solicit feedback more quickly. Additionally, it does mirror other libraries that follow similar patterns, like rectify. However, one thing I like about the symbols is that they they provide more context about what the method did rather than silently passing, raising an error, or true/false.

I actually do like failing noisily, so I don't mind rescuing and reraising if the Error is not what I expect it to be, but am wary of it until it's clear that we have a solid error reporting system in place that doesn't completely disrupt service to the end users.

Whenever i've done service objects I generally follow the call! pattern (cause I like to fail noisily). (There are times when i'd have different methods if different places need different data from the same source).

My thought process goes like this, the ! means it will raise some error on failure and it is your job as the caller to catch it and handle appropriately. If it returns nothing, that means the service succeeded in what it was doing (this can also mean the service didn't need to do anything because making them idempotent is nice).

As for reporting, yeah we should prob implement that at some point. I generally tend to use Sentry and have had good results.

Sidenote:

Now we could include Dry Structs/ Dry types or ways to returned typed data to keep things consistent but I still don't see much value adding more levels of complexity.

toyhammered · 2023-08-27T20:54:11Z

app/forms/profile_form.rb

@@ -0,0 +1,35 @@
+class ProfileForm


Given what I said above about extracting the resume to it's own service object, I don't think we really need this. Most the complications came because of that, and everything else is just simple rails form stuff which should be in the model.

How do you typically approach handling form data when it's not associated with a particular resource on the unrelated resource's controller? Do you namespace it differently in the form? Do you create a separate params method that requires those data, e.g.

def resume_params params.require(:profile).permit(:resume, :resume_name, :current_resume_id) end

I don't have too much of an opinion. separate params would be totally fine by me though

app/views/profiles/edit.html.erb

afogel · 2023-08-30T10:54:40Z

Responding to @toyhammered 's comments, I've now updated the PR to remove the form object. One outstanding concern I have is handling validation errors on the resume side of things, since that's not technically covered by the profile errors.
That said, service object encapsulates the more complex logic associated with updating a resume and is fully tested, so maybe that's less problematic.

I've also moved the business logic associating with updating the current profile and current resume into the profile controller and wrapped all of it in a single transaction block.

…ion gets rolled back

afogel · 2023-08-30T11:15:22Z

.rubocop.yml

@@ -307,6 +307,8 @@ Rails/HasAndBelongsToMany:
  Enabled: false
 Rails/TimeZone:
  Enabled: false
+Rails/HelperInstanceVariable:


Rubocop incorrectly identified the commands directory as a helper, even after I excluded using the following:

Rails/HelperInstanceVariable: Exclude: - "app/commands/**/**/*"

I've disabled for now.

afogel · 2023-08-30T11:24:54Z

spec/factories/profiles.rb

@@ -44,11 +44,11 @@
    end

    transient do
-      attached_picture { false }
+      picture { nil }


I discovered when I investigated further that the transient attribute is expecting the name of an attribute that is actually defined on the model. Since Profile defines picture, but not attached_picture, I've updated the factory and spec to reflect that change.

codecov · 2023-08-30T11:25:30Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.05% 🎉

Comparison is base (f14f5c0) 97.84% compared to head (8e6bf0c) 97.89%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #265      +/-   ##
==========================================
+ Coverage   97.84%   97.89%   +0.05%     
==========================================
  Files         165      167       +2     
  Lines        2131     2190      +59     
==========================================
+ Hits         2085     2144      +59     
  Misses         46       46

Files Changed	Coverage Δ
app/models/resume.rb	`100.00% <100.00%> (ø)`
app/models/user.rb	`90.90% <100.00%> (+0.28%)`	⬆️
app/services/resumes/update_service.rb	`100.00% <100.00%> (ø)`
spec/components/profile/picture_component_spec.rb	`100.00% <100.00%> (ø)`
spec/factories/profiles.rb	`100.00% <100.00%> (ø)`
spec/factories/resumes.rb	`100.00% <100.00%> (ø)`
spec/services/resumes/update_service_spec.rb	`100.00% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

toyhammered

Looking good, just couple small things!

app/models/user.rb

toyhammered · 2023-08-30T15:29:11Z

app/commands/resumes/update.rb

+module Resumes
+  class Update


Can we move this into a services instead of commands.

toyhammered · 2023-08-30T15:39:13Z

app/commands/resumes/update.rb

+      ActiveRecord::Base.transaction do
+        user.current_resume.update!(current: false)
+
+        if attributes[:resume]
+          user.resumes.create!(attributes)
+          next
+        end
+
+        user.resumes.find_by(id: current_resume_id).update!(current: true)
+      end


I think we can optimize this quickly a tiny bit. We should be able to next if attributes[:resume].blank? && current_resume.id == current_resume id.

Other than that this makes sense what is happening. Might be worth adding a small comment explaining that they can chose a different resume to make as current (took me a min to figure that out), and not need to upload one.

I think some comments might clarify the intention, as the current way its written makes the logic flow read confusingly.
next is not really a correct word for the context, but rubocop is irritating and i thin, in this case, wrong (Rails/TransactionExitStatement) rubocop/rubocop-rails#642. I can update to ignore rubocop in this context.

It should be this:

ActiveRecord::Base.transaction do user.current_resume.update!(current: false) return user.resumes.create!(attributes) if attributes[:resume] user.resumes.find_by(id: current_resume_id).update!(current: true) end

what rubocop is saying is correct. I understand the next and return difference. It was more asking if we could include a guard clause because user.current_resume.update!(current: false) and user.resumes.find_by(id: current_resume_id).update!(current: true) might be the exact same record which could already be true.

gotcha. updated.

toyhammered · 2023-08-30T15:43:20Z

app/controllers/profiles_controller.rb

+    if update_records
      redirect_to @profile, success: 'Profile successfully updated!'
    else
+      # using this now misses validations and errors on the resume part of the form.
      flash.now[:form_errors] = @profile.errors.full_messages
      render :edit, status: :unprocessable_entity
    end


I feel like it should always just redirect and the else can just happen if there is an error (i.e: rescue).

Can you clarify this, please?

this service isn't going to be returning anything. Wouldn't it just be nil or raising some type of error which you'd need to capture in the controller?

got it. yeah, fair enough.

add resume upload

b193d1a

afogel requested review from toyhammered, crespire and JoshDevHub August 27, 2023 16:48

afogel commented Aug 27, 2023

View reviewed changes

spec/commands/profiles_update_spec.rb Outdated Show resolved Hide resolved

Merge branch 'main' into resume_upload

914d8af

JoshDevHub reviewed Aug 27, 2023

View reviewed changes

app/views/profiles/edit.html.erb Outdated Show resolved Hide resolved

toyhammered requested changes Aug 27, 2023

View reviewed changes

afogel added 4 commits August 30, 2023 13:41

Merge branch 'main' into resume_upload; address PR comments

e38157f

linter fixes

fa1fe7d

wrap the entire update in a transaction

d968024

order resumes by id (e.g. upload date)

0efa094

afogel added 2 commits August 30, 2023 14:07

linter fixes

ef10c4b

enforce that the updating fails noisly to make sure database transact…

7beaa97

…ion gets rolled back

afogel commented Aug 30, 2023

View reviewed changes

afogel added 2 commits August 30, 2023 14:17

fix js linting

556088f

update picture component spec to conform with updated API for factory

97cb11c

afogel commented Aug 30, 2023

View reviewed changes

afogel added 5 commits August 30, 2023 15:08

remove unused let statement

d9692ea

actually, refactor for better clarity of command spec

ee66223

cleaner abstraction

521c8dd

linter god sacrifice

5adff51

make the tests work with the association scope

87577eb

toyhammered requested changes Aug 30, 2023

View reviewed changes

afogel added 4 commits August 31, 2023 08:17

respond to comments

6484b6f

Merge branch 'main' into resume_upload

17f7690

ensure resume attribute exists on resumes update service

64d1df7

more linter fixes

8e6bf0c

toyhammered approved these changes Aug 31, 2023

View reviewed changes

afogel merged commit 999029b into main Aug 31, 2023
3 checks passed

afogel deleted the resume_upload branch August 31, 2023 15:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add resume upload #265

add resume upload #265

afogel commented Aug 27, 2023

toyhammered left a comment

toyhammered Aug 27, 2023

afogel Aug 28, 2023

toyhammered Aug 29, 2023

toyhammered Aug 27, 2023

afogel Aug 28, 2023

toyhammered Aug 29, 2023

afogel commented Aug 30, 2023

afogel Aug 30, 2023

afogel Aug 30, 2023

codecov bot commented Aug 30, 2023 •

edited

Loading

toyhammered left a comment

toyhammered Aug 30, 2023

afogel Aug 30, 2023

toyhammered Aug 30, 2023

afogel Aug 30, 2023 •

edited

Loading

toyhammered Aug 30, 2023

afogel Aug 31, 2023

toyhammered Aug 30, 2023

afogel Aug 30, 2023

toyhammered Aug 30, 2023

afogel Aug 31, 2023

add resume upload #265

add resume upload #265

Conversation

afogel commented Aug 27, 2023

What's the change?

Highlights / Surprises / Risks / Cleanup

Demo / Screenshots

Issue ticket number and link

Checklist before requesting a review

toyhammered left a 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

Choose a reason for hiding this comment

afogel commented Aug 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 30, 2023 • edited Loading

Codecov Report

toyhammered left a 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

afogel Aug 30, 2023 • 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

codecov bot commented Aug 30, 2023 •

edited

Loading

afogel Aug 30, 2023 •

edited

Loading