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

Issue 188 - Return number of emails contacted #295

Merged
merged 29 commits into from
Nov 25, 2020
Merged

Issue 188 - Return number of emails contacted #295

merged 29 commits into from
Nov 25, 2020

Conversation

ijadams
Copy link
Contributor

@ijadams ijadams commented Oct 9, 2020

Description

Adding a field for returning the amount of emails the contact/send endpoint sends to

  • Fixing the parseAsync issue on download csv endpoint
  • Switching endpoint to download CSV in browser so FE doesn't have to handle it
  • Send single email for contact or entity

Related PRs

Related Front-End dependency -

CodeForBaltimore/Healthcare-Rollcall#198

@codecov
Copy link

codecov bot commented Oct 9, 2020

Codecov Report

Merging #295 (454b84a) into master (98924d4) will decrease coverage by 2.65%.
The diff coverage is 52.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #295      +/-   ##
==========================================
- Coverage   87.08%   84.43%   -2.66%     
==========================================
  Files          17       17              
  Lines         635      668      +33     
==========================================
+ Hits          553      564      +11     
- Misses         82      104      +22     
Impacted Files Coverage Δ
src/routes/contact.js 77.56% <33.33%> (-8.77%) ⬇️
src/routes/csv.js 75.00% <71.42%> (-12.50%) ⬇️
src/email/index.js 80.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 98924d4...454b84a. Read the comment docs.

src/routes/contact.js Outdated Show resolved Hide resolved
src/routes/contact.js Show resolved Hide resolved
src/routes/csv.js Outdated Show resolved Hide resolved
@blakenan-bellese
Copy link
Contributor

@ijadams Can you take a look at Michael S feedback and address/respond? Also, can you resolve conflicts? Looking to do housekeeping of PRs to merge/close outstanding PRs. Thanks.

@ijadams504
Copy link
Contributor

Maybe could be rebased onto Glazer's https://github.com/CodeForBaltimore/Bmore-Responsive/pull/307/files

@joshglazer
Copy link
Member

@mschechter-bellese @blakenan-bellese I've made the updates to the error codes that Michael suggested, and also cleaned up the merge conflicts. I'm not sure what to do about the auto-formatting/linting ... I'm trying to get eslint set up in my IDE but am having a hard time, is this something you all use or am I barking up the wrong tree?

})
} else {
response.setMessage('Email Address not found.')
response.setCode(500)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is a server error appropriate here? This is a data problem with the specific user.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

422 is definitely something we need to move away from; it's specific to WebDAV and should be only used in those types of cases. 500 is inappropriate because there was no server error at all.

In this case, there's nothing wrong with the provided entity (the request), and the user exists; they just don't have an e-mail address. It's not really clear what this should be because the field is optional. The closest one I can find is that because the resource has missing data we return a 404 with an explanation. This is a short-term solution, as the new API (I think) avoids this scenario entirely.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the feedback @mschechter-bellese .. I've changed this status code to 404.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants