224 confirmation email when companies register by FranciscoCardoso913 · Pull Request #306 · NIAEFEUP/nijobs-be

FranciscoCardoso913 · 2023-02-11T23:44:28Z

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:

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

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

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  )?

render · 2023-02-11T23:44:34Z

Your Render PR Server URL is https://nijobs-be-pr-306.onrender.com.

Follow its progress at https://dashboard.render.com/web/srv-cfk2gnhgp3jgtsvl5cu0.

FranciscoCardoso913 · 2023-02-27T21:29:59Z

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.

Naapperas · 2023-06-18T22:59:08Z

@FranciscoCardoso913 @dsantosferreira Updates on this?

+import { SECOND_IN_MS } from "../../models/constants/TimeConstants.js";
+
+export const exceededCreationTimeLimit = async (req, res, next) => {
+    const application = await CompanyApplication.findOne({ email: req.body.email, isVerified: false });


…ress Co-authored-by: Francisco Cardoso <FranciscoCardoso913@users.noreply.github.com>

Naapperas

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

codecov · 2023-08-23T17:31:56Z

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.

Co-authored-by: Francisco Cardoso <FranciscoCardoso913@users.noreply.github.com>

…lications Co-authored-by: Daniel Ferreira <dsantosferreira@users.noreply.github.com>

Co-authored-by: Daniel Ferreira <dsantosferreira@users.noreply.github.com>

…ication Co-authored-by: Daniel Ferreira <dsantosferreira@users.noreply.github.com>

Co-authored-by: Francisco Cardoso <FranciscoCardoso913@users.noreply.github.com>

…after validating the application

…ng of pending offers depending on application status Co-authored-by: Francisco Cardoso <FranciscoCardoso913@users.noreply.github.com>

…s approved Co-authored-by: Francisco Cardoso <FranciscoCardoso913@users.noreply.github.com>

…ers. Co-authored-by: Francisco Cardoso <FranciscoCardoso913@users.noreply.github.com>

Co-authored-by: Daniel Ferreira <dsantosferreira@users.noreply.github.com>

…ing the type of errors some functions throw Co-authored-by: Daniel Ferreira <dsantosferreira@users.noreply.github.com>

Co-authored-by: Daniel Ferreira <dsantosferreira@users.noreply.github.com>

…the auth/recover/:token/confirm endpoint Co-authored-by: Daniel Ferreira <dsantosferreira@users.noreply.github.com>

…plications/company/:id/reject

…plication Co-authored-by: Daniel Ferreira <dsantosferreira@users.noreply.github.com>

… to previously uncover parts

DoStini · 2023-11-02T09:31:14Z

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


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."

@FranciscoCardoso913 did you give an explanation regarding this?

DoStini · 2023-11-02T09:51:11Z

+        await Account.create({
            email,
-            password,
+            password: (password),


Why this change?

It was an accident

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

…one is updated instead of being deleted and replaced

+    async updateOrCreate(query, update) {
+        let application = await CompanyApplication.findOne(query);
+        if (!application) application = await this.create(update);
+        else application = await CompanyApplication.findOneAndUpdate(query, update, { new: true });


…irmation-email-when-companies-register

Naapperas

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 🥲)

Naapperas · 2024-03-14T03:03:29Z

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


@FranciscoCardoso913 did you give an explanation regarding this?

Naapperas · 2024-03-14T03:06:10Z

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?

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

Naapperas · 2024-03-14T03:09:43Z

     */
-    router.post("/", validators.create, async (req, res, next) => {
-
+    router.post("/", validators.create, applicationMiddleware.exceededCreationTimeLimit, async (req, res, next) => {


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.

Naapperas · 2024-03-14T03:10:24Z

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


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

Naapperas · 2024-03-14T03:16:04Z

+                const account = await Account.findOne({ company: req.targetOwner });
+                const application = await CompanyApplication.findOne({ email: account.email });


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.

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

Naapperas · 2024-03-14T03:17:29Z

            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 });


Same thing as last comment.

Naapperas · 2024-03-14T03:18:44Z

+<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>


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

Naapperas · 2024-03-14T03:19:52Z

+<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>


@FranciscoCardoso913 any comments on this?

Naapperas · 2024-03-14T03:21:41Z

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


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.

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

Naapperas · 2024-03-14T03:22:50Z

+        await Account.create({
            email,
-            password,
+            password: (password),


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

Naapperas · 2024-03-14T03:30:36Z

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.

DoStini

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

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

Confirmation Email when companies register #224

Open

FranciscoCardoso913 mentioned this pull request Feb 11, 2023

Validate company application page NIAEFEUP/nijobs-fe#309

Open

DoStini suggested changes Feb 12, 2023

View reviewed changes

dsantosferreira assigned dsantosferreira and FranciscoCardoso913 Feb 27, 2023

dsantosferreira force-pushed the 224-confirmation-email-when-companies-register branch from 896610a to 441c78e Compare March 7, 2023 14:03

FranciscoCardoso913 force-pushed the 224-confirmation-email-when-companies-register branch from 441c78e to 709babb Compare March 11, 2023 12:32

Naapperas mentioned this pull request Mar 21, 2023

Refactor Offer State #312

Open

Naapperas requested changes Apr 17, 2023

View reviewed changes

Comment thread src/services/company.js

Naapperas requested review from DoStini and Naapperas April 25, 2023 14:09

dsantosferreira force-pushed the 224-confirmation-email-when-companies-register branch from 96a71fb to dabb53b Compare June 21, 2023 09:23

github-advanced-security AI found potential problems Jun 21, 2023

View reviewed changes

Added time interval to add second application with the same email add…

e700271

…ress Co-authored-by: Francisco Cardoso <FranciscoCardoso913@users.noreply.github.com>

github-advanced-security AI found potential problems Jun 22, 2023

View reviewed changes

Comment thread src/services/company.js Fixed

Naapperas requested changes Jul 24, 2023

View reviewed changes

Comment thread src/api/routes/company.js Outdated

Comment thread test/end-to-end/company/:companyId/state.js

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 force-pushed the 224-confirmation-email-when-companies-register branch from 33d750f to 5d65bf9 Compare September 16, 2023 16:46

FranciscoCardoso913 requested a review from Naapperas September 16, 2023 19:37

dsantosferreira and others added 6 commits October 11, 2023 15:05

Moved time interval logic to validator

e3ba9c7

Co-authored-by: Francisco Cardoso <FranciscoCardoso913@users.noreply.github.com>

Sends an email to company to confirm application (not tested)

ba84863

Co-authored-by: Francisco Cardoso <FranciscoCardoso913@users.noreply.github.com>

Changing validators to middleware and creating routes to validate app…

3fd941b

…lications Co-authored-by: Daniel Ferreira <dsantosferreira@users.noreply.github.com>

Making the program send a confirmation email when registing

7fe08b5

Co-authored-by: Daniel Ferreira <dsantosferreira@users.noreply.github.com>

Fixing order in wich emails are sent to company when creating an appl…

ca019bb

…ication Co-authored-by: Daniel Ferreira <dsantosferreira@users.noreply.github.com>

Add unverified state

74c964a

Co-authored-by: Francisco Cardoso <FranciscoCardoso913@users.noreply.github.com>

FranciscoCardoso913 and others added 18 commits October 11, 2023 15:14

Changing the order when the account is created so that it is created …

a135fdf

…after validating the application

Fixed frontend confirmation link

ba32103

Created new isPending attribute in Offer model and implemented creati…

5625a90

…ng of pending offers depending on application status Co-authored-by: Francisco Cardoso <FranciscoCardoso913@users.noreply.github.com>

Set isPending of all company's offers to false when its application i…

bc95f0b

…s approved Co-authored-by: Francisco Cardoso <FranciscoCardoso913@users.noreply.github.com>

Get all currently active offers endpoint only returns non-pending off…

994509a

…ers. Co-authored-by: Francisco Cardoso <FranciscoCardoso913@users.noreply.github.com>

Fixed offers not updating isPending status after company is approved

46f4581

Route to get an application from a company

89b75e8

Co-authored-by: Daniel Ferreira <dsantosferreira@users.noreply.github.com>

Rewriting the emails to match the current flow.

96dfd74

Co-authored-by: Daniel Ferreira <dsantosferreira@users.noreply.github.com>

Adding and correcting tests for the endpoint apply/company

b12205c

Co-authored-by: Daniel Ferreira <dsantosferreira@users.noreply.github.com>

Adding tests for the endpoint apply/company/:token/validate and chang…

6dada6d

…ing the type of errors some functions throw Co-authored-by: Daniel Ferreira <dsantosferreira@users.noreply.github.com>

Adding tests for the endpoint /company/:companyId/state

3f60be8

Co-authored-by: Daniel Ferreira <dsantosferreira@users.noreply.github.com>

Created and fixed tests for the offers/ endpoint and fixed tests for …

482b015

…the auth/recover/:token/confirm endpoint Co-authored-by: Daniel Ferreira <dsantosferreira@users.noreply.github.com>

Fixed tests from token.js and from the review endpoints

5d95f3b

Added tests for the endpoints applications/company/:id/approve and ap…

e113bc8

…plications/company/:id/reject

Changed endpoint that returned company's state to return company's ap…

64b8c31

…plication Co-authored-by: Daniel Ferreira <dsantosferreira@users.noreply.github.com>

Clear unused functions and unneeded verifications and made more tests…

96719ac

… to previously uncover parts

Updating the API documentation

43a1e44

Improved applications' endpoints documentation

b193933

FranciscoCardoso913 force-pushed the 224-confirmation-email-when-companies-register branch from 5d65bf9 to b193933 Compare October 12, 2023 10:13

Fixed netlify issue

711626e

DoStini suggested changes Nov 2, 2023

View reviewed changes

FranciscoCardoso913 added 2 commits February 5, 2024 22:34

When creating a new application if an old application exists the old …

f7fd7e3

…one is updated instead of being deleted and replaced

Updating email texts and error types

46d86fd

github-advanced-security AI found potential problems Feb 5, 2024

View reviewed changes

FranciscoCardoso913 requested a review from DoStini February 21, 2024 15:56

Merge branch 'develop' of github.com:NIAEFEUP/nijobs-be into 224-conf…

4f35451

…irmation-email-when-companies-register

FranciscoCardoso913 force-pushed the 224-confirmation-email-when-companies-register branch from df467c5 to 4f35451 Compare March 13, 2024 23:50

Naapperas requested changes Mar 14, 2024

View reviewed changes

DoStini approved these changes Apr 3, 2024

View reviewed changes

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

		const account = await Account.findOne({ company: req.targetOwner });
		const application = await CompanyApplication.findOne({ email: account.email });

Conversation

FranciscoCardoso913 commented Feb 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Confirmation email when companies register

Important:

Discussed solution:

Back-End changes:

Emails new order:

What is yet to be done:

Doubts:

Uh oh!

render Bot commented Feb 11, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

FranciscoCardoso913 commented Feb 27, 2023

New workflow

Discussed solution

Uh oh!

Uh oh!

Naapperas commented Jun 18, 2023

Uh oh!

Check failure

Uh oh!

Uh oh!

Naapperas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Aug 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Check failure

Naapperas left a comment

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!

FranciscoCardoso913 commented Feb 11, 2023 •

edited

Loading

codecov Bot commented Aug 23, 2023 •

edited

Loading