a13xh7 / QaraTMS

QaraTMS is open source test case, test suites, test plans and test runs management tool.
MIT License
62 stars 19 forks source link

Discussion: Refactoring of the Code Base #41

Closed ENG3PLabs closed 2 weeks ago

ENG3PLabs commented 2 weeks ago

Disclaimer: I am quite new to Laravel. Everything I say or propose here is based on limited knowledge and is in no way meant to criticize the current code base since I do not know how it and Laravel evolved over time.

While looking into the project and trying to figure out what does what (not just for the apps logic but Laravel in general), I noticed a few things that made it hard for me to get into it. I mainly noticed these things as I read through a lot of the Laravel documentation, but also while trying to translate knowledge from other technologies into Laravel so I could understand the logic behind the framework and the app.

My background here is largely React, basic PHP, and the Spring framework. These help with understanding:

Now there were a few things that threw me a little under the bus or that seemed to make the code base harder to understand than it needs to be. I want to list these things here and open a discussion (if needed) about whether or not we could change a few things. The goal is to declutter the code and align it a little bit more with what I found to be the best practices displayed inside the Laravel docs.

Models outside of Models Namespace

image

Usually, models reside inside the app/Models directory and are also generated there, if the Models directory is there.

Proposal: Move the models inside a new app/Models directory.

Unused Routes for the API, Channels, and Commands

image

There are these resources that are not used by the project, and in the case of api.php, even seem to provide an endpoint that maybe should not be made available.

Proposal: remove these resources.

Pages and Requests use Inconsistent Routes and have Custom Names

This is a bigger one 😬

There are two things I noticed here:

Route Definition

The route names like project_list_page do not correspond with the ones proposed by the routers for resources from Laravel. According to them, the name for the / route would be projects.index instead of project_list_page.

Furthermore, I was wondering, why there were specific routes for all the basic functions like, index, create, show, edit, store, update, and delete. Route::resource('projects', ProjectController::class) would also generate these and work with the controllers just fine. Strangely enough, this is documented under "Controllers" and not "Routes" and can be found here: Laravel 11 Doc - Resource Controllers.

These controllers also work for nested resources as well as for resources like user, that do not want to provide all routes.

The reason why I would like to change this is that some of the names make more sense when viewed together with the actual URL that they belong to. So index instead of list_page. Also, it would highly reduce the size and perceived complexity of the web.php file.

Route URLs

There is an inconsistent use between singular and plural resource names in the URLs e.g. /users , /test-plans and /documents but then /project and /test-run. And there are short-cut resource names like /tc and /tse.

I'd like to follow REST standards here, which would also be already enforced when using resource controllers.

Proposal: Replace the route definitions by using Route::resource and update the route() and redirect() calls to use the new route names. This should probably be done on a resource-by-resource basis to keep the PR sizes manageable.

Split Create and Edit Blade Templates

I personally think that it is more cumbersome to keep two different templates alive rather than having one work for newly created and already present resources.

Proposal: Combine the edit and create pages for the resources with conditional rendering of the labels and actions.

Fin

Although I may have forgotten things, these are my main concerns and while working on them, other things would probably pop up again so I could document or fix them there.

I'm curious about the feedback on this and am looking forward to continue my work on this project.

a13xh7 commented 2 weeks ago

@ENG3PLabs I am a QA engineer and I have little experience with Laravel and js so my code is far from perfect.

you can refactor the project's code.

ENG3PLabs commented 2 weeks ago

Thank you! Will continue then.

a13xh7 commented 2 weeks ago

@ENG3PLabs Hi, I reverted last 2 commits. You broke test case creating and other staff, buttons to create or edit was hidden or removed.

a13xh7 commented 2 weeks ago

or something is wrong with opera browser

ENG3PLabs commented 2 weeks ago

Thanks for the heads-up. I will be checking it. Also I just noticed the Discussions-tab. Sorry I didn't put this there.

Let's maybe keep discussions on the PRs so that the info relating to them is in a single place.