Skip to content
This repository has been archived by the owner on Dec 14, 2023. It is now read-only.

Make zone serial increment properly #141

Closed
wants to merge 1 commit into from

Conversation

gadall
Copy link
Contributor

@gadall gadall commented Oct 29, 2021

Zone serial was resetting (decrementing) when saving zone, and failing to increment when editing/adding/removing a record.

// We need a new one ONLY if there isn't pending changes.
if ($this->hasPendingChanges() && ! $force) {
return $currentSerial;
if (isset($this->serial)) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen you're addressing the use case when the serial is empty. Would you be so kind to add a test for this specific case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pacoorozco This became a problem because I changed ZoneController::fillZoneFromRequest to call getNewSerialNumber() instead of assign $zone->serial = Zone::generateSerialNumber(). This introduced the case of ! isset($this->serial).
The reason for that change was that this assignment was re-setting and decrementing serial numbers when the zone was being saved.

I'm not too familiar with Laravel etc., I see right now you're doing:
$zone = factory(Zone::class)->create();
$expectedSerial = $zone->serial;
and that worked, so I suppose this creates a zone with a pre-seeded serial?
So the test you're asking for would be more like:
$zone = new Zone();
$zone->getNewSerialNumber();
$this->assertGreaterThan(0, $zone->serial);

@pacoorozco
Copy link
Owner

Hi @gadall
Thanks a lot for raising this PR. I've just suggested to add a test to cover the use case that you have found.

@gadall gadall force-pushed the fix-serial branch 2 times, most recently from f3637ec to 0635978 Compare October 30, 2021 20:57
@pacoorozco
Copy link
Owner

@gadall This should be fixed in the current 1.x version (main branch)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants