Refactor createTemplateService and add a few automated tests #507

anarute · 2021-08-24T22:33:28Z

This PR should be read as a proposal for a bigger architectural refactor.

Since I need to work on a big feature, I really want us to cover the project with automated tests but to achieve this goal we need to do some refactoring.

I used the createTemplateService as the first gnea pig since I have worked a bit with it and it is a bit independent.

I'm still not 100% happy with the name TemplateService and even if it should be inside the services folder, but for now maybe it's enough.

My idea is to slowly try to apply DDD in the project, but I want to be conservative for now to avoid breaking things by mistake.

In summary in this PR:

Remove the test folder and all the existing tests (they didn't seem to be testing anything really)
Refactor createTemplatesService to call a new class TemplateService that tries to break down a bit how we are parsing the template creation request
Add 2 initial tests to test the TemplateService class
Introduce namespace to the projects (I'm still learning so I know it can be better hehe )

jaragunde · 2021-08-25T10:30:03Z

Hi Ana, thanks for this hard work. I need to set aside some time to go through your proposal, but I have one comment.

Could the new autoload setup be used to simplify the usage of require/include we do in the codebase? If you take a look, you will find that any user-facing PHP pages define this constant pointing to the root directory:

define('PHPREPORT_ROOT', __DIR__ . '/../../');

Then, every include uses it like:

include_once(PHPREPORT_ROOT . '/model/vo/TemplateVO.php');

So, with autoload and namespaces, could we stop using PHPREPORT_ROOT? Notice I'm a bit lost about modern PHP practices, too :-)

anarute · 2021-08-25T12:52:58Z

Hi Ana, thanks for this hard work. I need to set aside some time to go through your proposal, but I have one comment.

Could the new autoload setup be used to simplify the usage of require/include we do in the codebase? If you take a look, you will find that any user-facing PHP pages define this constant pointing to the root directory:
define('PHPREPORT_ROOT', __DIR__ . '/../../');
Then, every include uses it like:
include_once(PHPREPORT_ROOT . '/model/vo/TemplateVO.php');
So, with autoload and namespaces, could we stop using PHPREPORT_ROOT? Notice I'm a bit lost about modern PHP practices, too :-)

That's a good point, I'll create an autoload file and use it instead of the one generated by composer so we don't need to use compose to deploy to production yet, only to run the automated tests locally. This will be enough for us to clean up these include lines :) I'll work on it next week (I'll be off for a few days)

anarute · 2021-08-25T12:54:17Z

And yes we'll be able to stop using PHPREPORT_ROOT 🎉

jaragunde

I agree with the general direction in this PR, and I'm eager to see how we can share more code between different services, add more tests, etc.

I just have a few comments below:

jaragunde · 2021-08-31T11:04:37Z

web/services/TemplateService.php

+    private \LoginManager $loginManager;
+
+    public function __construct(
+        \TemplateVO $templateVO,


The TemplateVO is a Value Object, just a representation of the data for a template in an object form. It's a bit confusing to have one TemplateVO attached to the TemplateService and reuse it, I would create a new object for every template we parse.

This will create an object every time, but now it is created outside the class. The idea is to use dependency injection to make the class more independent and easier to isolate and test.

But for TemplateVo you are probably right that I don't need to do this since it's very isolated already, I'll change this and pass only the LoginManager.

I've been reading about dependency injection and, while I can see the advantage of injecting the LoginManager, I'm not sure about the TemplateVO. I wonder what's the usual procedure: inject all classes, decide case-by-case... The main difference for me is that, while LoginManager provides a service, TemplateVO acts more like a basic data type.

This will create an object every time, but now it is created outside the class.

I see, now I realize the template create operation only supports creating one template for every operation. But, I wonder how DI could be achieved if the service supported creating several at the same time, like the task create service does.

jaragunde · 2021-08-31T11:05:47Z

web/services/TemplateService.php

+        $createTemplates = [];
+        do {
+            if ($parser->name == "template") {
+                $templatesVO = $this->templateVO;


Just create a new TemplateVO every time.

jaragunde · 2021-08-31T11:12:32Z

.gitignore

@@ -18,3 +18,5 @@ config/config.defaults
 #minified JS and source maps
 *.min.js
 *.min.js.map
+
+vendor


We can commit this as it is now, but could you please configure your editor to add newline characters at the end of the file?

jaragunde · 2021-08-31T11:14:31Z

composer.lock

+        "This file locks the dependencies of your project to a known state",
+        "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies",
+        "This file is @generated automatically"
+    ],


This file is huge and contains lots of dependencies... Where do they come from? Are they all really needed?

Yes :/ they are all sub dependencies of phpunit/phpunit.
Note that for production there are no dependencies needed, only for dev (under packages-dev line 9)

jaragunde · 2021-08-31T11:21:29Z

web/services/TemplateService.php

+class TemplateService
+{
+    private \TemplateVO $templateVO;
+    private \LoginManager $loginManager;


At first, I didn't understand why you were storing an instance of LoginManager instead of using it in a static way :)

Maybe add a comment explaining that this allows to fake the login manager for testing?

Yes, will do! I'll try to break this down into smaller commits

jaragunde · 2021-08-31T15:10:47Z

That's a good point, I'll create an autoload file and use it instead of the one generated by composer so we don't need to use compose to deploy to production yet, only to run the automated tests locally. This will be enough for us to clean up these include lines :) I'll work on it next week (I'll be off for a few days)

I finally understood what you meant when I read the last commit (I hadn't noticed it when I wrote the review above).

If I understand correctly, it's a temporary solution to prevent composer from being compulsory, right? If that's the case, I would even prefer to start using composer and the improved autoload at the same time and skip the temporary situation.

anarute · 2021-08-31T16:55:11Z

That's a good point, I'll create an autoload file and use it instead of the one generated by composer so we don't need to use compose to deploy to production yet, only to run the automated tests locally. This will be enough for us to clean up these include lines :) I'll work on it next week (I'll be off for a few days)

I finally understood what you meant when I read the last commit (I hadn't noticed it when I wrote the review above).

If I understand correctly, it's a temporary solution to prevent composer from being compulsory, right? If that's the case, I would even prefer to start using composer and the improved autoload at the same time and skip the temporary situation.

Yes, it would be an intermediate step, ok I'll remove this commit and keep composer :)

Tests is more used and intuitive since it stores all the tests not just one. Also the existing test files weren't working and only had tests for getters and setters (probably auto generated tests).

The createTemplateService will just receive a request and return whatever the TemplateService parses and executes from it. The goal is to start splitting what handles the requests and what processes the data and business logic.

Add parseTemplates that is a pure function that receives a XMLReader and returns an array of TemplateVO with the parsed values from the XML.

Use depedency injection to decouple TemplateService from LoginManager. This will make TemplateService more isolated and easier to test, since with this change we are able to mock LoginManager instead of instanciating it.

Add initial set of tests for the TemplateService: one for testing the parserTemplates method and another to check if user can't create a template if there's no active section

anarute · 2021-08-31T19:46:14Z

@jaragunde I broke down the commits trying to make the change in steps for a better review. Can you review the whole PR again please?

Sorry for making it confusing the first time 😅

About the usage of PHPREPORT_ROOT, we'll be able to replace all the requires and includes by namespace and use and do a unique include require PHPREPORT_ROOT . "vendor/autoload.php"; in some strategic files, but this would require to modify almost every file in the repository, that's why I haven't done yet. I think it's better if we start using composer and make sure this patch works in beta and prod, then I can do for the rest of the project.

jaragunde · 2021-09-02T08:53:32Z

I think it's better if we start using composer and make sure this patch works in beta and prod, then I can do for the rest of the project.

I agree, let's take small steps.

jaragunde

Wow, you really walked the extra mile to reorganize and tidy up the commits! I wouldn't have dared to ask that much 😄

Everything looks great now!

jaragunde · 2021-09-09T13:28:25Z

docs/developer/devel-setup.rst

+
+Step 6: Running automated tests
+===========================
+


make help complains this underline is too short. There's no need to go back to the list of commits and fix this, let's amend it later.

oh my bad, I didn't notice it had to match the amount of characters of the title

I didn't know, either! I thought it was enough with three. But this is not actually Markdown syntax so 🤷

jaragunde · 2021-09-09T13:31:32Z

Note (to self): composer is packaged in Fedora and Debian (probably Ubuntu, I guess), the package name is composer. We probably could mention that in the instructions, like we mention package names for other dependencies. I can do that later.

On the other hand, we might just distribute the phpreport releases with the autoload already generated and dependencies included and mention composer only as a development dependency. Let's figure this out later, when we discuss future releases... Maybe it's not even worth to keep doing them!

jaragunde · 2021-09-15T07:52:35Z

On the other hand, we might just distribute the phpreport releases with the autoload already generated and dependencies included and mention composer only as a development dependency. Let's figure this out later, when we discuss future releases... Maybe it's not even worth to keep doing them!

I did one last release without the composer dependency, in the old style. I think that, in the future, it's not worth to keep doing these releases, they are a lot of manual, error-prone work. I've opened #511 to discuss that.

anarute force-pushed the add-first-unit-test branch from fb32e7d to 80d93f4 Compare August 30, 2021 19:41

jaragunde reviewed Aug 31, 2021

View reviewed changes

anarute force-pushed the add-first-unit-test branch from 80d93f4 to 62640b2 Compare August 31, 2021 19:20

anarute added 7 commits August 31, 2021 16:25

Remove test folder and create tests

3954015

Tests is more used and intuitive since it stores all the tests not just one. Also the existing test files weren't working and only had tests for getters and setters (probably auto generated tests).

Add composer and autoload settings

55b4b2b

Move template creation logic to TemplateService

74a3e2a

The createTemplateService will just receive a request and return whatever the TemplateService parses and executes from it. The goal is to start splitting what handles the requests and what processes the data and business logic.

Split template creation

864c208

Add parseTemplates that is a pure function that receives a XMLReader and returns an array of TemplateVO with the parsed values from the XML.

Pass LoginManager as a parameter instead of instanciating it

6107705

Use depedency injection to decouple TemplateService from LoginManager. This will make TemplateService more isolated and easier to test, since with this change we are able to mock LoginManager instead of instanciating it.

Add tests for TemplateService

e40028d

Add initial set of tests for the TemplateService: one for testing the parserTemplates method and another to check if user can't create a template if there's no active section

Add automated tests instructions

21fd68d

anarute force-pushed the add-first-unit-test branch from 62640b2 to 21fd68d Compare August 31, 2021 19:26

jaragunde approved these changes Sep 2, 2021

View reviewed changes

jaragunde reviewed Sep 9, 2021

View reviewed changes

anarute mentioned this pull request Sep 13, 2021

Add Holiday Management view #512

Merged

jaragunde merged commit 37ee590 into Igalia:main Sep 15, 2021

anarute deleted the add-first-unit-test branch September 15, 2021 11:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor createTemplateService and add a few automated tests #507

Refactor createTemplateService and add a few automated tests #507

anarute commented Aug 24, 2021

jaragunde commented Aug 25, 2021

anarute commented Aug 25, 2021

anarute commented Aug 25, 2021

jaragunde left a comment

jaragunde Aug 31, 2021

anarute Aug 31, 2021

jaragunde Sep 2, 2021

jaragunde Aug 31, 2021

jaragunde Aug 31, 2021

jaragunde Aug 31, 2021

anarute Aug 31, 2021

jaragunde Aug 31, 2021

anarute Aug 31, 2021

jaragunde commented Aug 31, 2021

anarute commented Aug 31, 2021

anarute commented Aug 31, 2021

jaragunde commented Sep 2, 2021

jaragunde left a comment

jaragunde Sep 9, 2021

anarute Sep 9, 2021

jaragunde Sep 9, 2021

jaragunde commented Sep 9, 2021

jaragunde commented Sep 15, 2021

Refactor createTemplateService and add a few automated tests #507

Refactor createTemplateService and add a few automated tests #507

Conversation

anarute commented Aug 24, 2021

jaragunde commented Aug 25, 2021

anarute commented Aug 25, 2021

anarute commented Aug 25, 2021

jaragunde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaragunde commented Aug 31, 2021

anarute commented Aug 31, 2021

anarute commented Aug 31, 2021

jaragunde commented Sep 2, 2021

jaragunde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaragunde commented Sep 9, 2021

jaragunde commented Sep 15, 2021