diff --git a/libraries/src/MVC/Controller/ApiController.php b/libraries/src/MVC/Controller/ApiController.php index e123e70177796..55583dc71b60e 100644 --- a/libraries/src/MVC/Controller/ApiController.php +++ b/libraries/src/MVC/Controller/ApiController.php @@ -416,13 +416,29 @@ public function edit() throw new NotAllowed('JLIB_APPLICATION_ERROR_CREATE_RECORD_NOT_PERMITTED', 403); } + // A check-out this user already holds is not this request's to release. + $checkedOutField = $table->getColumnAlias('checked_out'); + $alreadyCheckedOut = $table->hasField('checked_out') + && $table->load($recordId) + && (int) $table->$checkedOutField > 0; + // Attempt to check-out the new record for editing and redirect. if ($table->hasField('checked_out') && !$model->checkout($recordId)) { // Check-out failed, display a notice but allow the user to see the record. throw new Exception\CheckinCheckout(Text::sprintf('JLIB_APPLICATION_ERROR_CHECKOUT_FAILED', $model->getError())); } - $this->save($recordId); + try { + $this->save($recordId); + } catch (\Exception $e) { + // The save was refused: release the check-out if this request took it. + if (!$alreadyCheckedOut) { + $model->checkin($recordId); + } + + throw $e; + } + $this->displayItem($recordId); return $this; diff --git a/tests/System/integration/api/com_content/Articles.cy.js b/tests/System/integration/api/com_content/Articles.cy.js index 709c8eb15a7a1..0fe23738059df 100644 --- a/tests/System/integration/api/com_content/Articles.cy.js +++ b/tests/System/integration/api/com_content/Articles.cy.js @@ -88,6 +88,24 @@ describe('Test that content API endpoint', () => { .should('include', 'updated automated test article')); }); + it('does not leave an article checked out when an update is refused', () => { + cy.db_createArticle({ title: 'automated test article' }) + .then((article) => cy.api_getBearerToken() + .then((token) => cy.request({ + method: 'PATCH', + url: `/api/index.php/v1/content/articles/${article.id}`, + body: { title: '' }, + headers: { Authorization: `Bearer ${token}` }, + json: true, + failOnStatusCode: false, + })) + .then((response) => { + expect(response.status).to.be.at.least(400); + cy.task('queryDB', `SELECT checked_out FROM #__content WHERE id = ${article.id}`) + .then((rows) => expect(Number(rows[0].checked_out ?? 0)).to.equal(0)); + })); + }); + it('can delete an article', () => { cy.db_createArticle({ title: 'automated test article', state: -2 }) .then((article) => cy.api_delete(`/content/articles/${article.id}`)); diff --git a/tests/Unit/Libraries/Cms/MVC/Controller/ApiControllerTest.php b/tests/Unit/Libraries/Cms/MVC/Controller/ApiControllerTest.php new file mode 100644 index 0000000000000..0f197a920b399 --- /dev/null +++ b/tests/Unit/Libraries/Cms/MVC/Controller/ApiControllerTest.php @@ -0,0 +1,204 @@ + + * @license GNU General Public License version 2 or later; see LICENSE.txt + */ + +namespace Joomla\Tests\Unit\Libraries\Cms\MVC\Controller; + +use Joomla\CMS\Application\CMSWebApplicationInterface; +use Joomla\CMS\MVC\Controller\ApiController; +use Joomla\CMS\MVC\Factory\MVCFactoryInterface; +use Joomla\Input\Input; +use Joomla\Tests\Unit\UnitTestCase; +use PHPUnit\Framework\Attributes\TestDox; + +/** + * Test class for \Joomla\CMS\MVC\Controller\ApiController + * + * @package Joomla.UnitTest + * @subpackage MVC + * + * @since __DEPLOY_VERSION__ + */ +#[TestDox('The ApiController')] +class ApiControllerTest extends UnitTestCase +{ + /** + * @return void + * + * @since __DEPLOY_VERSION__ + */ + #[TestDox('releases the check-out when an edit is refused')] + public function testRefusedEditChecksTheRecordBackIn() + { + $model = $this->modelDouble(); + $controller = $this->controller($model, new \RuntimeException('Save failed')); + + try { + $controller->edit(); + $this->fail('The refusal must still reach the client'); + } catch (\RuntimeException $e) { + $this->assertSame('Save failed', $e->getMessage()); + } + + $this->assertSame([42], $model->checkedOut); + $this->assertSame([42], $model->checkedIn); + } + + /** + * @return void + * + * @since __DEPLOY_VERSION__ + */ + #[TestDox('leaves a successful edit to save()\'s own check-in')] + public function testSuccessfulEditIsUnchanged() + { + $model = $this->modelDouble(); + $controller = $this->controller($model, null); + + $controller->edit(); + + $this->assertSame([42], $model->checkedOut); + $this->assertSame([], $model->checkedIn, 'edit() adds no check-in of its own on success'); + } + + /** + * @return void + * + * @since __DEPLOY_VERSION__ + */ + #[TestDox('keeps a check-out the user already held when an edit is refused')] + public function testRefusedEditKeepsAnExistingCheckOut() + { + $model = $this->modelDouble(7); + $controller = $this->controller($model, new \RuntimeException('Save failed')); + + try { + $controller->edit(); + $this->fail('The refusal must still reach the client'); + } catch (\RuntimeException $e) { + $this->assertSame('Save failed', $e->getMessage()); + } + + $this->assertSame([42], $model->checkedOut); + $this->assertSame([], $model->checkedIn, 'The check-out was not taken by this request'); + } + + /** + * A model double that records check-out and check-in calls. + * + * @param integer $checkedOutBy The user already holding the check-out (0 for none) + * + * @return object + * + * @since __DEPLOY_VERSION__ + */ + private function modelDouble(int $checkedOutBy = 0): object + { + return new class ($checkedOutBy) { + public array $checkedOut = []; + public array $checkedIn = []; + + public function __construct(private int $checkedOutBy) + { + } + + public function getTable() + { + return new class ($this->checkedOutBy) { + public function __construct(public int $checked_out) + { + } + + public function load($pk) + { + return true; + } + + public function getColumnAlias($name) + { + return $name; + } + + public function hasField($name) + { + return $name === 'checked_out'; + } + + public function getKeyName() + { + return 'id'; + } + }; + } + + public function checkout($pk) + { + $this->checkedOut[] = (int) $pk; + + return true; + } + + public function checkin($pk) + { + $this->checkedIn[] = (int) $pk; + + return true; + } + }; + } + + /** + * An ApiController editing record 42, whose save() throws $refusal (or succeeds when null). + * + * @param object $model The model double + * @param \Throwable|null $refusal What save() throws + * + * @return ApiController + * + * @since __DEPLOY_VERSION__ + */ + private function controller(object $model, ?\Throwable $refusal): ApiController + { + $config = ['base_path' => __DIR__, 'option' => 'com_content']; + $input = new Input(['id' => 42]); + + return new class ($config, $this->createStub(MVCFactoryInterface::class), $this->createStub(CMSWebApplicationInterface::class), $input, $model, $refusal) extends ApiController { + protected $contentType = 'articles'; + + public function __construct($config, $factory, $app, $input, private object $modelDouble, private ?\Throwable $refusal) + { + parent::__construct($config, $factory, $app, $input); + } + + public function getModel($name = '', $prefix = '', $config = []) + { + return $this->modelDouble; + } + + protected function allowEdit($data = [], $key = 'id') + { + return true; + } + + protected function save($recordKey = null) + { + if ($this->refusal) { + throw $this->refusal; + } + + return $recordKey; + } + + public function displayItem($id = null) + { + return $this; + } + }; + } +}