nextcloud / forms

📝 Simple form & survey app for Nextcloud
https://apps.nextcloud.com/apps/forms
GNU Affero General Public License v3.0
324 stars 98 forks source link

chore: Refactor lastUpdated #2314

Closed Chartman123 closed 1 month ago

Chartman123 commented 2 months ago

This commit refactors the form creation and update logic in the ApiController class. It removes the unnecessary setting of the created and lastUpdated timestamps in the Form entity, as these values are now automatically set in the FormMapper class. This improves code readability and reduces redundancy.

The changes also include updates to the FormMapper class, where the insert and update methods now automatically set the created and lastUpdated timestamps respectively.

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 21.62162% with 29 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@b40ef76). Learn more about missing BASE report. Report is 2 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2314 +/- ## ======================================= Coverage ? 36.25% Complexity ? 1011 ======================================= Files ? 70 Lines ? 3801 Branches ? 0 ======================================= Hits ? 1378 Misses ? 2423 Partials ? 0 ```
Chartman123 commented 2 months ago

@susnux do you have any idea why I get an Error 500 as soon as I open the Forms app?

Type: Error
Code: 0
Message: Xdebug has detected a possible infinite loop, and aborted your script with a stack depth of '512' frames
File: /srv/www/htdocs/nextcloud/lib/private/AppFramework/Utility/SimpleContainer.php
Line: 104
Chartman123 commented 2 months ago

@susnux I was able to resolve my issue (thanks to @come-nc)

This can now be reviewed. I currently needed a little hack in the ApiControllerTest to make them pass. (state and lastUpdated don't have the expected values in line 499. null instead of 0 and 0 instead of 123456789).

Chartman123 commented 1 month ago

@Koc thanks for the approval. Do you perhaps have an idea how to fix the test in a better way? I'm no expert in tests 🙈😂

Koc commented 1 month ago

Usually it is recommended to rely on Psr\Clock\ClockInterface instead of plain time() and use MockClock implementation in tests.

Another variant - you can mock native time() by defining mocked function inside same namespace.

But I think we can live with current approach.

Chartman123 commented 1 month ago

Yes we can leave it for now and add a to-do comment. What bothers me the most is that the time() mocking worked before but doesn't anymore. Probably because the timestamp generation is now moved from the ApiController to the FormMapper