Skip to content
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

Barcode and labelling depending on Sample Type #607

Merged
merged 23 commits into from
Feb 8, 2018

Conversation

Espurna
Copy link

@Espurna Espurna commented Jan 26, 2018

Description of the issue/feature this PR addresses

The current system allows the user to select the barcode template to be used in a preview before
labels being printed.

A mechanism that will automatically choose the template in the preview and in accordance with the
sample type will be added.

Current behavior before PR

All registered stickers are displayed on preview load.

Desired behavior after PR is merged

Once the barcode labels preview is loaded, the system will look for the
sample types and will update the template accordingly. A selection list will be added in the Sample Type edit view too: this will allow the user to set the template to be rendered for each Sample Type.

--
I confirm I have tested this PR thoroughly and coded it according to PEP8
and Plone's Python styleguide standards.

@xispa xispa changed the title Barcode and labelling deppending on Sample Type Barcode and labelling depending on Sample Type Jan 26, 2018
Copy link
Member

@xispa xispa left a comment

Choose a reason for hiding this comment

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

Some changes required in favour of complexity reduction and clearlyness

default = self.sample_type.getDefaultSmallSticker()
else:
default = self.sample_type.getDefaultLargeSticker()
return default
Copy link
Member

Choose a reason for hiding this comment

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

IMO, would be better like

size = self.request.get('size', '')
if size == 'small':
    return self.sample_type.getDefaultSmallSticker()
return self.sample_type.getDefaultLargeSticker()

adapters = getAdapters((self.context, ), IGetStickerTemplates)
except ComponentLookupError:
logger.info('No IGetStickerTemplates adapters found.')
adapters = None
Copy link
Member

Choose a reason for hiding this comment

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

no need to do the assignment adapters = None here, just return list()

Copy link
Author

@Espurna Espurna Feb 2, 2018

Choose a reason for hiding this comment

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

The method continues as follows: if there aren't adapters nor templates, get all sticker templates in the system. So we can't return at this point.

for name, adapter in adapters:
templates += adapter(self.request)
if templates:
return templates
Copy link
Member

Choose a reason for hiding this comment

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

Reduce nesting:

if not adapters:
    return list()
templates = list()
for name, adapter in adapters:
    templates.append(adapter(self.request))
if templates:
   ...

Copy link
Author

@Espurna Espurna Feb 2, 2018

Choose a reason for hiding this comment

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

As I said in a previous comment, I can't return until the end of the method.

Moreover, adapters variable can be a generator object or None. There could be the case where the generator contains "nothing", like an empty list. So, if getAdapters doesn't fail, adapters variable will be an object even and consequently it will always be true.

voc.add(sticker.get('id'), sticker.get('title'))
if voc.index > 0:
return voc
return DisplayList()
Copy link
Member

Choose a reason for hiding this comment

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

no need to check if voc.index > 0, just do return voc

subfield_vocabularies={
'admitted': sticker_templates(),
'small_default': '_small_default_voc',
'large_default': '_large_default_voc',
Copy link
Member

Choose a reason for hiding this comment

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

No need of two different functions _small_default_voc and _large_default_voc for the same logic, just create a function get_sticker_templates_vocabularies() and use it instead.

'AdmittedStickerTemplates',
subfields=(
'admitted',
'small_default',
Copy link
Member

Choose a reason for hiding this comment

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

Why small_default and large_default instead of small and large?

Since you use these strings in other places from the same class, better to define two global variable STICKER_SMALL = 'small_default' and STICKER_LARGE = 'large_default' and use them everywhere.

Copy link
Author

Choose a reason for hiding this comment

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

small_default and large_default instead of small and large because from my point of view those are more descriptives names. As an example the description for small_default "Default small sticker"

if not values:
return []
admitted = values[0].get('admitted')
return admitted
Copy link
Member

Choose a reason for hiding this comment

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

Almost the same exact logic as getDefaultSmallSticker and getDefaultLargeSticker. Better to create the following functions:

def getDefaultSticker(self, size=STICKER_SMALL):
    return self.get_sticker_subfield(size)

def getAdmittedStickers(self):
    return self._get_sticker_subfield('admitted', default=list())

def _get_sticker_subfield(subfield, default=None):
    if subfield is None:
        return default
    values = self.getAdmittedStickerTemplates()
    if not values:
        return default
    return values[0].get('admitted', default)    

voc.add(sticker.get('id'), sticker.get('title'))
if voc.index > 0:
return voc
return DisplayList()
Copy link
Member

Choose a reason for hiding this comment

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

Same exact function as _small_default_voc. Please use just only one function get_sticker_templates_vocabularies() as suggested in a comment above

field = self.getField('AdmittedStickerTemplates')
new_value = field.get(self)
new_value[0]['large_default'] = value
field.set(self, new_value)
Copy link
Member

Choose a reason for hiding this comment

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

Again, the code of setDefaultLargeSticker is almost exact to setDefaultSmallSticker, please use a single function e.g. setDefaultSticker(self, value, SIZE=STICKER_SMALL)

field = self.getField('AdmittedStickerTemplates')
new_value = field.get(self)
new_value[0]['admitted'] = value
field.set(self, new_value)
Copy link
Member

Choose a reason for hiding this comment

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

Same reasoning done for getAdmittedStickers above. Do something like this:

def setDefaultSticker(self, size=STICKER_SMALL):
    if not size or size not in [STICKER_SMALL, STICKER_LARGE]:
        return
    return self._set_sticker_subfield(size)

def setAdmittedStickers(self, value):
    if not value or not isinstance(value, list):
        return
    return self._set_sticker_subfield('admitted', default=list())

def _set_sticker_subfield(subfield, value):
    field = self.getField('AdmittedStickerTemplates')
    stickers = field.get(self)
    stickers[0][subfield] = value
    field.set(self, stickers)

@xispa xispa merged commit 00232f7 into senaite:master Feb 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants