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

Added Search Functionality to Get All Companies Endpoint #42

Merged

Conversation

obafemitayor
Copy link
Contributor

@obafemitayor obafemitayor commented Dec 8, 2024

Related Issue

Related PRs

What Changed

  • Added search functionality to the get all companies endpoint here
  • Did some Minor refactoring on the endpoint, made it more RESTful
  • Renamed the /company/stack/all endpoint to follow standard REST guidelines. Confirmed that this would not cause any breaking changes on the front end.

Testing Strategy

  • Integration Tests. Use this command to run the test php artisan test --filter=CompanyTest, for running the test in docker use docker-compose run app php artisan test --filter=CompanyTest
  • Testing Evidence
image image image

routes/api.php Outdated Show resolved Hide resolved
routes/api.php Outdated Show resolved Hide resolved
Copy link
Contributor Author

@obafemitayor obafemitayor Dec 8, 2024

Choose a reason for hiding this comment

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

Not the most ideal way to do an integration test. In order to achieve full end to end testing, the database should not be mocked, but as the project itself has no test coverage, I guess this could suffice.

@obafemitayor
Copy link
Contributor Author

Hello @jovialcore, can you help take a look when you have the chance?.

@obafemitayor
Copy link
Contributor Author

@jovialcore I guess I will need to rebase from main and fix the conflicts before you can have a look at this one. I would let you know once this is done

@jovialcore
Copy link
Owner

@jovialcore I guess I will need to rebase from main and fix the conflicts before you can have a look at this one. I would let you know once this is done

Aite.

@obafemitayor
Copy link
Contributor Author

@jovialcore I guess I will need to rebase from main and fix the conflicts before you can have a look at this one. I would let you know once this is done

Aite.

@jovialcore I have merge this PR with all the latest changes from master and it is now ready for a review. Here is also the related PR for the frontend changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not the most ideal way to do an integration test. In order to achieve full end to end testing, the database should not be mocked, but as the project itself has no test coverage, I guess this could suffice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not the most ideal way to do an integration test. In order to achieve full end to end testing, the database should not be mocked, but as the project itself has no test coverage, I guess this could suffice.

@@ -6,27 +6,30 @@
use App\Http\Resources\CompanyResource;
use App\Models\Company;

use App\Services\SearchService;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this as it is not used anywhere in this code

@jovialcore
Copy link
Owner

Alright. Will review. I appreciate it, @obafemitayor .

@jovialcore jovialcore merged commit 65af7d6 into jovialcore:main Dec 10, 2024
1 check failed
@jovialcore
Copy link
Owner

Hi @obafemitayor thanks for the PR. I usually do some shoutout on X and LinkedIn. Do you mind if I get your @ for X and LinkedIn?

@obafemitayor
Copy link
Contributor Author

Hi @obafemitayor thanks for the PR. I usually do some shoutout on X and LinkedIn. Do you mind if I get your @ for X and LinkedIn?

Hello @jovialcore my LinkedIn is https://www.linkedin.com/in/obafemi-tayo-6296b28b

@obafemitayor
Copy link
Contributor Author

Hello @jovialcore, I wanted to ask if you have deployed the latest backend changes to master. I visited the site now and I am seeing this

image

@jovialcore
Copy link
Owner

Yeah. I am on it. seen that. been having issues with deploying. but I am on it

@jovialcore
Copy link
Owner

@obafemitayor Fixed. It's done now. You can check again

@obafemitayor
Copy link
Contributor Author

@obafemitayor Fixed. It's done now. You can check again

@jovialcore Yes it works now. Waiting for your Linkedin SO 😉😉

@jovialcore
Copy link
Owner

Sure sure 👏🏽😄. Thank you.

@jovialcore jovialcore mentioned this pull request Dec 11, 2024
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.

3 participants