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

224 confirmation email when companies register #306

Open
wants to merge 31 commits into
base: develop
Choose a base branch
from

Conversation

FranciscoCardoso913
Copy link
Contributor

@FranciscoCardoso913 FranciscoCardoso913 commented Feb 11, 2023

Confirmation email when companies register

Important:

Although this is a back-end issue it also has changes in the Front-End! See the changes made in the Front-End here: NIAEFEUP/nijobs-fe#309

Issue: #224

Discussed solution:

-When created, applications should have a state UNVERIFIED by default instead of the current PENDING state;

-After creating an application an email with a link should be sent to the company so they can validate their application, the link should expire in 10 minutes;

-A company should not be able to create an application with an email associated with an UNVERIFIED application whose link is yet to expire;

-If the link of an application expires and the company tries to create another application the previous application of that company should be deleted;

-The link should send the user to a page where the application is validated, when validating the application its state should change to pending and an email should be sent to the company.

Back-End changes:

-The application model now has a new attribute called "isVerified" which indicates if the application has already been validated or not by the company, the attribute is set to false when the application is created;

-There is a new application state, an application is UNVERIFIED when the isVerified is false, and PENDING when isVerified is true and both of the approvedAt and the rejectedAt are null;

-Before creating a new application the Back-End now checks if an application with the same email as the new application had been created in less than 10 minutes from the time the request for a new application is sent, if so the Back-End returns an error to the Front-End, otherwise, the Back-End deletes the old application( if it exists) and creates a new one;

-If the Front-End requests it the Back-End validates the application, returning an error if for some reason the validation is illegal (token has expired, the token is not valid, the application is already validated, etc...) ;

-Emails have been changed! Now when a new application is created a single email with a link is sent to the company, only after the application is validated that two other emails are sent, one of them sent to the NIJobs team informing that a company has created a new application and the other sent to the company to inform that everything went well and that we will review their request.

-The function verifyAndDecodeToken now returns a different error when the token expires and when the token is invalid, unlike before when the error returned was the same;

Emails new order:

-The first email which is sent to the company when the application is created:

Screenshot from 2023-02-11 23-06-16

-The second email sent to the NiJobs team when the application is validated.

Screenshot from 2023-02-11 23-07-30

-The third email which is sent to the company after the application is validated.

Screenshot from 2023-02-11 23-08-37

What is yet to be done:

-Tests;

Doubts:

-Should we change any of the email's text/order?

-Did we use the API errors correctly or should we have done something differently?

-Is everything in the correct place or is there anything that is more adequate somewhere else?

-Should the UNVERIFIED applications, which can no longer be verified, be deleted automatically from time to time or should we let it be like it is now (it's only deleted when a new application with the same email is created  )?

@FranciscoCardoso913 FranciscoCardoso913 linked an issue Feb 11, 2023 that may be closed by this pull request
@render
Copy link

render bot commented Feb 11, 2023

.env Outdated Show resolved Hide resolved
.env Outdated Show resolved Hide resolved
src/lib/token.js Outdated Show resolved Hide resolved
src/models/CompanyApplication.js Outdated Show resolved Hide resolved
src/models/CompanyApplication.js Outdated Show resolved Hide resolved
src/api/middleware/auth.js Outdated Show resolved Hide resolved
src/api/middleware/application.js Outdated Show resolved Hide resolved
src/api/middleware/application.js Outdated Show resolved Hide resolved
src/api/middleware/application.js Outdated Show resolved Hide resolved
src/api/routes/application.js Outdated Show resolved Hide resolved
@FranciscoCardoso913
Copy link
Contributor Author

New workflow

Discussed solution

  • When creating a new application companies will need to verify their account by clicking on a link sent by email.
  • After that, the company will be able to finish its registration. In addition to that, an email will be sent to the NiJobs team to inform us that a new company wants to register so that we can evaluate its application
  • After finishing their registration, companies will be able to create offers, however, these offers will only be made public when the company's application is accepted (its status will be "pending")
  • When the company is accepted by us it will have full access to the NiJobs website.

@dsantosferreira dsantosferreira force-pushed the 224-confirmation-email-when-companies-register branch from 896610a to 441c78e Compare March 7, 2023 14:03
@FranciscoCardoso913 FranciscoCardoso913 force-pushed the 224-confirmation-email-when-companies-register branch from 441c78e to 709babb Compare March 11, 2023 12:32
src/services/company.js Outdated Show resolved Hide resolved
@Naapperas
Copy link
Member

@FranciscoCardoso913 @dsantosferreira Updates on this?

@dsantosferreira dsantosferreira force-pushed the 224-confirmation-email-when-companies-register branch from 96a71fb to dabb53b Compare June 21, 2023 09:23
src/api/middleware/application.js Dismissed Show dismissed Hide dismissed
src/services/application.js Fixed Show fixed Hide fixed
src/services/company.js Fixed Show fixed Hide fixed
Copy link
Member

@Naapperas Naapperas left a comment

Choose a reason for hiding this comment

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

Regarding the check annotations, GH does not have the ability to do an E2E security audit, from the looks of it. Most of the times the security issues that it signals are already being mitigated my validators/middleware. We should still see if these are valid just in case something slips through

src/api/routes/company.js Outdated Show resolved Hide resolved
test/end-to-end/company/:companyId/state.js Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 23, 2023

Codecov Report

Attention: Patch coverage is 90.82569% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 90.55%. Comparing base (82886d0) to head (4f35451).

Files Patch % Lines
src/services/application.js 88.23% 4 Missing ⚠️
src/api/routes/application.js 83.33% 2 Missing ⚠️
src/api/routes/company.js 77.77% 2 Missing ⚠️
src/services/company.js 60.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #306      +/-   ##
===========================================
+ Coverage    90.34%   90.55%   +0.21%     
===========================================
  Files           54       55       +1     
  Lines         1470     1546      +76     
  Branches       246      257      +11     
===========================================
+ Hits          1328     1400      +72     
- Misses         137      141       +4     
  Partials         5        5              

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

@dsantosferreira dsantosferreira force-pushed the 224-confirmation-email-when-companies-register branch 2 times, most recently from 5e73233 to 0be0cf9 Compare September 10, 2023 22:46
@FranciscoCardoso913 FranciscoCardoso913 force-pushed the 224-confirmation-email-when-companies-register branch from 33d750f to 5d65bf9 Compare September 16, 2023 16:46
FranciscoCardoso913 and others added 18 commits October 11, 2023 15:14
…ng of pending offers depending on application status

Co-authored-by: Francisco Cardoso <[email protected]>
…ing the type of errors some functions throw

Co-authored-by: Daniel Ferreira <[email protected]>
…the auth/recover/:token/confirm endpoint

Co-authored-by: Daniel Ferreira <[email protected]>
@FranciscoCardoso913 FranciscoCardoso913 force-pushed the 224-confirmation-email-when-companies-register branch from 5d65bf9 to b193933 Compare October 12, 2023 10:13
src/api/middleware/application.js Show resolved Hide resolved
return next();
} catch (jwtErr) {
if (jwtErr.name === "TokenExpiredError") {
return next(new APIError(HTTPStatus.GONE, ErrorTypes.FORBIDDEN, ValidationReasons.EXPIRED_TOKEN));
Copy link
Member

Choose a reason for hiding this comment

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

Why gone? This should just be forbidden, no?

"The HyperText Transfer Protocol (HTTP) 410 Gone client error response code indicates that access to the target resource is no longer available at the origin server and that this condition is likely to be permanent."

Copy link
Member

Choose a reason for hiding this comment

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

@FranciscoCardoso913 did you give an explanation regarding this?

src/api/routes/application.js Outdated Show resolved Hide resolved
src/email-templates/approval_notification.handlebars Outdated Show resolved Hide resolved
src/email-templates/approval_notification.handlebars Outdated Show resolved Hide resolved
src/lib/token.js Show resolved Hide resolved
src/services/application.js Show resolved Hide resolved
email,
password,
password: (password),
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was an accident

Copy link
Member

Choose a reason for hiding this comment

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

@FranciscoCardoso913 this does not seem to have changed, was it a typo or not?

src/services/application.js Outdated Show resolved Hide resolved
src/models/CompanyApplication.js Outdated Show resolved Hide resolved
src/services/application.js Dismissed Show dismissed Hide dismissed
@FranciscoCardoso913 FranciscoCardoso913 force-pushed the 224-confirmation-email-when-companies-register branch from df467c5 to 4f35451 Compare March 13, 2024 23:50
Copy link
Member

@Naapperas Naapperas left a comment

Choose a reason for hiding this comment

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

Most of the comments I made serve as introductions for discussion, not necessarily requested changes. There are some, however, hence this.

Besides this, I just remembered something: what is the behavior of offers belonging to a company that gains full access to NIJobs if there are more than the allowed amount of concurrent offers pending? (I might have forgotten, sorry 🥲)

return next();
} catch (jwtErr) {
if (jwtErr.name === "TokenExpiredError") {
return next(new APIError(HTTPStatus.GONE, ErrorTypes.FORBIDDEN, ValidationReasons.EXPIRED_TOKEN));
Copy link
Member

Choose a reason for hiding this comment

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

@FranciscoCardoso913 did you give an explanation regarding this?

Copy link
Member

Choose a reason for hiding this comment

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

I just thought of something: although the token was generated as part of the process of a company applying to use NIJobs, the token itself is not part of the company. As such, I would suggest restructuring the code a bit, following the comments I'll be leaving next.

The general idea is that a token is part of an application but not a company, so having /apply/company/:token seemed a bit odd to me. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to ignore the following comments if you don't agree with this opinion, given that you reason about it.

@@ -11,15 +14,36 @@ export default (app) => {
/**
* Creates a new Company Application
*/
router.post("/", validators.create, async (req, res, next) => {

router.post("/", validators.create, applicationMiddleware.exceededCreationTimeLimit, async (req, res, next) => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
router.post("/", validators.create, applicationMiddleware.exceededCreationTimeLimit, async (req, res, next) => {
router.post("/company", validators.create, applicationMiddleware.exceededCreationTimeLimit, async (req, res, next) => {

I cannot edit unmodified lines but above, on line 12, it should become:

app.use("/apply", router);

This change should not break existing functionality/tests.

/**
* Validates application
*/
router.post("/:token/validate", validators.finishValidation, validToken, async (req, res, next) => {
Copy link
Member

Choose a reason for hiding this comment

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

All tests that hit this route should take into account the change made on the comment above.

Comment on lines +104 to +105
const account = await Account.findOne({ company: req.targetOwner });
const application = await CompanyApplication.findOne({ email: account.email });
Copy link
Member

Choose a reason for hiding this comment

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

On way for this to "look cleaner" is if the "pending" state could be computed from the account object itself. Take a look at Mongoose Schema Virtuals and see if you agree with this.

Copy link
Member

Choose a reason for hiding this comment

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

Also, maybe this could be hidden behind a service call? Don't we have an AccountService ?

@@ -82,7 +83,9 @@ export default (app) => {
async (req, res, next) => {

try {
const { account } = await (new ApplicationService()).approve(req.params.id);
const account = await (new ApplicationService()).approve(req.params.id);
const company = await Company.findOne({ _id: account.company });
Copy link
Member

Choose a reason for hiding this comment

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

Same thing as last comment.

<h1>We are delighted to have you on board, {{companyName}}!</h1>
<p>We are pleased to inform you that your application has been accepted, and all the offers
you have created so far are now visible to the public!</p>
<p>Going forward, any offer you create will be immediately visible to the public audience.</p>
Copy link
Member

Choose a reason for hiding this comment

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

I believe that, although companies should already know this, we should warn them about our imposed limits on concurrent offers.

Comment on lines 2 to 5
<p>We will now review your application to determine the trustworthiness of your company.
In the meantime, you can complete your registration by logging into your account and begin creating offers.
Please note that your offers will remain hidden from the public until your company is approved.</p>
<p>Your Application ID is {{applicationId}} and you registered {{companyName}}.</p>
Copy link
Member

Choose a reason for hiding this comment

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

@FranciscoCardoso913 any comments on this?

return null;
}
};
export const verifyAndDecodeToken = (token, secret) => jwt.verify(token, secret, { algorithm: "HS256" });
Copy link
Member

Choose a reason for hiding this comment

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

Why did you move the error handling logic out of this method? I recall you returning different errors based on the thrown exception. Couldn't we abstract those jwt exceptions into our custom, more semantic, ones? Food for thought.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with this comment, I think the error handling error should be inside and outside a simpler null check

email,
password,
password: (password),
Copy link
Member

Choose a reason for hiding this comment

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

@FranciscoCardoso913 this does not seem to have changed, was it a typo or not?

@Naapperas
Copy link
Member

Some of the comments appear outdated and I don't know why: when I open the respective files nothing changed. Please resolve or answer the conversations as if they were not outdated.

Copy link
Member

@DoStini DoStini left a comment

Choose a reason for hiding this comment

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

As of my comments, everything seems to be fine. Before merging address the comments @Naapperas suggested

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.

Confirmation Email when companies register
4 participants