247 default order offers by most recent

[dsantosferreira]

Closes #247

Offers are now ordered with the following precedence: score > publish date (by most recent) > id. I ran the test below using Postman to check if it was working as intended (using the query "Julia"):

[BrunoRosendo]

@dsantosferreira Run npm audit fix and see if the audit can be fixed automatically with it. I think it can

[dsantosferreira]

@dsantosferreira Run npm audit fix and see if the audit can be fixed automatically with it. I think it can

@BrunoRosendo Ran that command locally and it didn't find any vulnerabilities after fixing. However, it now has conflicts with branch "develop". What should i do?

[Naapperas]

@dsantosferreira Run npm audit fix and see if the audit can be fixed automatically with it. I think it can

@BrunoRosendo Ran that command locally and it didn't find any vulnerabilities after fixing. However, it now has conflicts with branch "develop". What should i do?

#278 fixes audit issues, maybe we can merge this PR after that one.

[BrunoRosendo]

@dsantosferreira You should remove the last commit. Check git rebase -i or git reset for example

It seems the audit is fixed in another PR so you can wait for it to be merged and rebase.

A note for the future, you can test the audit by running npm audit --production before pushing

[BrunoRosendo]

@dsantosferreira What's missing in this PR? Removing last commit and rebasing?

[BrunoRosendo]

I'm pretty sure this change breaks the pagination of the search. First of all, please read this PR and this article that explains the implementation we use in NIJobs.

We use a key-set based search, meaning we use IDs (or any field we use for sorting, like score or now publishDate). Why use IDs in the first place? Because they're inherently sorted by creation date. However, our system has a publishDate that might differ in order, and this PR will fix that.

That's where the problem occurs. For example, the user fetches 3 offers with id 1, 2 and 3, but they are actually now sorted by [3, 2, 1]. Rightly so, the system will register the last ID as 1 and create the queryToken. When the user fetches the next page, the system will retrieve that ID and return offers after ID 1 (instead of 3), meaning that offers 2 and 3 will be back in the second page 😨

Why isn't this tested? Because the pagination tests only use pages with size 1, so order does not matter. PLEASE, create tests for this scenario when you fix it!

Here's a summary of what should be done in order to fix this issue:

• Include the publishDate of the last offer in the queryToken. Look at decodeQueryToken and encodeQueryToken (after Make the search system sortable by other fields #221 it will be any sortable field)
• In _buildSearchContinuationQuery, change the logic a little bit:
  • When a value exists, retrieve offers before publish date if the score is the same. If both are the same, use the current ID system (do not remove this part).
  • When a value is not used, retrieve offers before the last publish date. If they're the same, use the ID once again

I think that should fix the issue. Once again, do not forget tests for these scenarios

[codecov-commenter]

Codecov Report

Base: 90.22% // Head: 90.10% // Decreases project coverage by -0.12% ⚠️

Coverage data is based on head (00bc403) compared to base (ceb5ef2).
Patch coverage: 77.77% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #276      +/-   ##
===========================================
- Coverage    90.22%   90.10%   -0.12%     
===========================================
  Files           54       54              
  Lines         1401     1405       +4     
  Branches       224      226       +2     
===========================================
+ Hits          1264     1266       +2     
- Misses         132      134       +2     
  Partials         5        5              

Impacted Files	Coverage Δ
src/api/middleware/validators/offer.js	95.91% <60.00%> (-1.29%)	⬇️
src/services/offer.js	99.05% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[Naapperas]

LGTM, only thing missing is testing this behavior correctly

[Naapperas]

@dsantosferreira #278 was merged, if you rebase it should clear most of the audit issues.

[BrunoRosendo]

@dsantosferreira There is a test failing and the PR has some commits from previous merges, can you fix that? Tell us if you need help

[dsantosferreira]

@dsantosferreira There is a test failing and the PR has some commits from previous merges, can you fix that? Tell us if you need help

@BrunoRosendo Regarding the commits from previous merges, I have already talked with Nuno and already know how to fix it. Unfortunately i won't be able to do it today but the issue should be fixed tomorrow night. As for the test that is failing I have no idea why that is happening... The test that fails comes from the "creating a new offer" endpoint which has nothing to do with the code I developed. While I was writing my tests I ran the test suites multiple times and noticed that some of the tests fail in an inconsistent way. I ran the test suites thrice just now and got these results:

First try

Second try

Third try

Any idea on how to solve this?

[Naapperas]

@dsantosferreira There is a test failing and the PR has some commits from previous merges, can you fix that? Tell us if you need help

@BrunoRosendo Regarding the commits from previous merges, I have already talked with Nuno and already know how to fix it. Unfortunately i won't be able to do it today but the issue should be fixed tomorrow night. As for the test that is failing I have no idea why that is happening... The test that fails comes from the "creating a new offer" endpoint which has nothing to do with the code I developed. While I was writing my tests I ran the test suites multiple times and noticed that some of the tests fail in an inconsistent way. I ran the test suites thrice just now and got these results:

First try

Second try

Third try

Any idea on how to solve this?

The inconsistent test results can be due to the way we have designed our tests, which will change after #169 is closed.

[render]

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

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

[dsantosferreira]

@dsantosferreira There is a test failing and the PR has some commits from previous merges, can you fix that? Tell us if you need help

The commits issue seems to be solved and the all tests passed this time!

[BrunoRosendo]

looks good, I just have a small question so I'm approving

[Naapperas]

LGTM

[BrunoRosendo]

@dsantosferreira This just needs a rebase and then we can merge

[dsantosferreira]

@dsantosferreira This just needs a rebase and then we can merge

Done.

[Naapperas]

@dsantosferreira this was failing the build tests: probably due to them being "bad-ish". When that happens you can re-run the GH workflow and see if you get them all passing, which allows for the PR to be merged.

[dsantosferreira]

@dsantosferreira this was failing the build tests: probably due to them being "bad-ish". When that happens you can re-run the GH workflow and see if you get them all passing, which allows for the PR to be merged.

Ok got it! Should I merge this PR?

[Naapperas]

@dsantosferreira this was failing the build tests: probably due to them being "bad-ish". When that happens you can re-run the GH workflow and see if you get them all passing, which allows for the PR to be merged.

Ok got it! Should I merge this PR?

Absolutely, it was already approved. Good job :)