New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Accession counter incrementing fix, refs #11700 #681
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mcantelon! Some comments bellow.
// If no identifier has been specified, use next available one | ||
if (!$this->identifier) | ||
// If identifier has been specified and the mask is enabled, increment the counter | ||
if (!empty($this->identifier) && QubitAccession::maskEnabled()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use self
instead of QubitAccession
like inside the if
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, true! I'll fix.
{ | ||
$this->identifier = self::nextAvailableIdentifier(); | ||
self::incrementAccessionCounter(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may need to wrap the incrementAccessionCounter
function content in a transaction too as it may be called from CLI in here after this changes, and the transaction in nextAvailableIdentifier
won't be working in that case.
public static function maskEnabled() | ||
{ | ||
$setting = QubitSetting::getByName('accession_mask_enabled'); | ||
return (null === $setting || boolval($setting->getValue(array('sourceCulture' => true)))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels wrong to me, are we sure that if the setting is missing (a rare case) we should consider that the mask is enabled? Otherwise, we could use sfConfig to get its value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This setting doesn't get created by a migration, alas, and the default behaviour IIRC is to use the mask.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, nice! Thanks
Added helper function to check whether mask's enabled and fixed inconsistent accession counter incrementing. Changes to logic relating to accession identifiers during database insert: * Removed automatic population of identifier, via the mask, if no accession identifier is specified * Added logic to, if an identifier is specified and mask is enabled, increment the accession counter
1e1b71a
to
35649a9
Compare
@jraddaoui I've wrapped the increment in a transaction and tested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mcantelon!
Added helper function to check whether mask's enabled and fixed inconsistent
accession counter incrementing.
Changes to logic relating to accession identifiers during database insert:
identifier is specified
the accession counter