Migrate Feedback Service

[skellamp]

TODO

• Create DoctrineTrait before testing TagService.

[demiankatz]

Thanks, @skellamp, this is excellent work and looks very good to me. I haven't done hands-on testing yet, but I have a few suggestions based on a review of the code. Once these changes are addressed, I'll try interactive testing and will let you know if I run into any problems there.

Beyond that, a couple of general thoughts/suggestions:

1.) You should probably start to delete the Laminas\Db code related to Feedback -- if all references to a function have been converted to Doctrine, we should delete that function. If all functions from a class have been removed, we should delete that class and remove it from plugin manager configuration. This will make it easier to keep track of which work still remains to be done.

2.) It may be worth thinking about whether any of the functions in FeedbackService would benefit from being refactored to use more generalized support methods in AbstractService. (I'm thinking about things like fetching a record by ID, updating a single column, etc.). This may not be worth doing right away, but if we find ourselves following the same pattern in every service class, a shared support method might reduce code and improve maintainability.

[demiankatz]

Thanks for the latest round of updates, @skellamp. This continues to look good! Just a few more minor suggestions. I think I'll be ready to test everything hands-on after your next update.

Speaking of testing, do you think it's worth starting to add some unit tests to this code now? It would be nice if we could start increasing test coverage as we work our way through the refactoring. Of course, if you're uncertain about the stability of any parts of the design, we can defer testing until we're confident that we won't have to redesign tests in the near future.

[demiankatz]

Thanks, @skellamp -- a few more minor suggestions!

[demiankatz]

@skellamp, I did some testing of this, and I'm encountering problems. I think the issue has to do with the way Doctrine is writing to the json field (form_data) in the database.

Here's what I did:

1.) I spun up a test instance of the dev branch, turned on database writing for the feedback form, and submitted a variety of feedback scenarios (logged in / logged out, with various blank fields, etc.).

2.) Next, I checked out your Feedback branch and ran composer install to update the dependencies, and cleared the local cache to avoid problems. I then submitted exactly the same feedback examples again.

3.) Now I turned on the admin module and looked at the feedback screen. I got a fatal error. I then switched back to dev and ALSO got a fatal error there.

4.) On inspection, it appears that the dev code is writing the JSON to the database correctly, but the Doctrine code is double-encoding the JSON, so it's getting stored as a string containing JSON, instead of as raw JSON. See my test data:

mysql> select form_data from feedback;

+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| form_data                                                                                                                                                                                           
+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| {"name": "", "email": "", "useragent": "Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Firefox/102.0"}                                                                          |
| {"name": "my name", "email": "", "useragent": "Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Firefox/102.0"}                                                                   |
| {"name": "my name", "email": "email@example.com", "useragent": "Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Firefox/102.0"}                                                  |
| {"name": "Demian Katz", "email": "demiankatz@gmail.com", "useragent": "Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Firefox/102.0"}                                           |
| {"name": "Demian Katz (edited)", "email": "demiankatz-edited@gmail.com", "useragent": "Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Firefox/102.0"}                           |
| "{\"name\":\"\",\"email\":\"\",\"useragent\":\"Mozilla\\/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko\\/20100101 Firefox\\/102.0\"}"                                                           |
| "{\"name\":\"Me Doctrine\",\"email\":\"\",\"useragent\":\"Mozilla\\/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko\\/20100101 Firefox\\/102.0\"}"                                                |
| "{\"name\":\"Me Doctrine\",\"email\":\"doctrin@example.com\",\"useragent\":\"Mozilla\\/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko\\/20100101 Firefox\\/102.0\"}"                             |
| "{\"name\":\"\",\"email\":\"\",\"useragent\":\"Mozilla\\/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko\\/20100101 Firefox\\/102.0\"}"                                                           |
| "{\"name\":\"Demian Katz\",\"email\":\"demiankatz@gmail.com\",\"useragent\":\"Mozilla\\/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko\\/20100101 Firefox\\/102.0\"}"                            |
| "{\"name\":\"Demian Katz (doctrine edited)\",\"email\":\"demiankatz.doctrine@gmail.com\",\"useragent\":\"Mozilla\\/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko\\/20100101 Firefox\\/102.0\"}" |
+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+

As you can see, the first 5 rows were written by the existing code, and the remaining rows by Doctrine. I think the difference is pretty obvious. :-)

Can you investigate whether there's a way to normalize this? JSON fields are kind of a newer feature in databases, so maybe Doctrine doesn't handle this gracefully... We'll need to come up with some kind of workaround if necessary, but hopefully it's just a minor configuration or escaping bug... :-)

[demiankatz]

Also, on an unrelated note, I tried to merge the latest dev branch into the doctrine branch and discovered that we've run into a dependency conflict between php-cs-fixer and the Laminas Doctrine module. It's being actively discussed at doctrine/DoctrineModule#800, but until that gets sorted out, we're going to be unable to resolve the current conflicts between dev and vufind-org#2233.

[demiankatz]

@skellamp, thanks for making progress on the JSON issue. I'm no longer seeing fatal errors, but there are still several problems (some related to JSON, others probably separate issues).

1.) JSON data is still being written to the database inconsistently. Data created with the dev branch is native JSON, but data created with this branch is a JSON string. This appears to be causing a problem where the "Show" pop-up in the "Additional data" column only includes full details for rows created by the dev code. The name, email and useragent values are missing in rows created by this branch.

2.) The "Form name" and "Site URL" filter drop-downs at the top of the admin page are not populating in this branch. In the dev branch, in the test scenario I described above, they each contain a value -- form name is "All" or "FeedbackSite" and site URL is "All" or "http://localhost/vufind_test". In this branch, both drop-downs only contain "All."

3.) Pagination is not working correctly. I created 13 example feedback submissions, and in the dev branch, these all display on a single page. In this branch, only the first 10 display, but then trying to switch to page 2 has no effect.

Can you please investigate these issues, and let me know if I can do anything to help! Sorry that this is proving a little more challenging than expected -- I had forgotten about the JSON column and should have realized that would add some complexity to the task!

[skellamp]

@skellamp, thanks for making progress on the JSON issue. I'm no longer seeing fatal errors, but there are still several problems (some related to JSON, others probably separate issues).

1.) JSON data is still being written to the database inconsistently. Data created with the dev branch is native JSON, but data created with this branch is a JSON string. This appears to be causing a problem where the "Show" pop-up in the "Additional data" column only includes full details for rows created by the dev code. The name, email and useragent values are missing in rows created by this branch.

2.) The "Form name" and "Site URL" filter drop-downs at the top of the admin page are not populating in this branch. In the dev branch, in the test scenario I described above, they each contain a value -- form name is "All" or "FeedbackSite" and site URL is "All" or "http://localhost/vufind_test". In this branch, both drop-downs only contain "All."

3.) Pagination is not working correctly. I created 13 example feedback submissions, and in the dev branch, these all display on a single page. In this branch, only the first 10 display, but then trying to switch to page 2 has no effect.

Can you please investigate these issues, and let me know if I can do anything to help! Sorry that this is proving a little more challenging than expected -- I had forgotten about the JSON column and should have realized that would add some complexity to the task!

The JSON field worked fine when tested on feedback branch. I was testing with a clean instance of database that might be the reason I never encountered the issues. All the three issues are now fixed. Please test it, hopefully we will not run into any other issues.

[demiankatz]

Thanks, @skellamp! It looks like all of the outstanding problems are now fixed. During the course of testing this, I found a couple of bugs in the original version in the dev branch. I've opened PRs vufind-org#2742 and vufind-org#2743 to fix them. You had already sorted out the first problem (broken pagination) here. I've cherry-picked my second fix into this branch to keep things in sync.

I'm going to merge this now! Let me know if you need suggestions for next steps; I know at least that it's on your list to refactor the tests as noted in the TODO above.