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

Refactor ajaxGetImportTemplate to find instrument import templates in addons #1349

Merged
merged 10 commits into from
May 20, 2019

Conversation

mikejmets
Copy link
Contributor

Description of the issue/feature this PR addresses

Linked issue: #1348

Current behavior before PR

Custom instrument import templates defined in addons are not found

Desired behavior after PR is merged

Custom instrument import templates defined in addons are used correctly

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

@mikejmets mikejmets requested a review from xispa April 29, 2019 15:33
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.

Thanks @mikejmets!. One change regarding getAnalysisServicesDisplayList function

brains = bsc(portal_type='AnalysisService')
items = []
for item in brains:
items.append((item.UID, item.Title))
Copy link
Member

Choose a reason for hiding this comment

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

As far as I am concerned, this function is used by some instrument interfaces (e.g. Qubit, Symex XS, etc.) to know the analysis services to which the results have to be applied. With this change, the value of the display list becomes the UID of the service instead of the keyword, so the parsers will probably not work properly:

analysis = request.form.get('analysis_service', None)
if analysis:
# Get default result key
defaultresult = request.form.get('default_result', None)
# Rise an error if default result is missing.
parser = SysmexXS500iCSVParser(infile, analysis, defaultresult) if defaultresult \
else errors.append(t(_("You should introduce a default result key.",
mapping={"fileformat": fileformat})))

Setup catalog has getKeyword metadata, so I think the following would be fine:

items = map(lambda item: (item.getKeyword, item.Title), brains)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The change I made here was purely flake8 cosmetics, the previous version also returned UID and Title.

Copy link
Member

Choose a reason for hiding this comment

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

Hi @mikejmets , apologies for my slow reply. I've been out for two weeks.
Ummm... don't think so. Note the "ugly" previous implementation was using getObject().Keyword:

items = [('', '')] + [(o.getObject().Keyword, o.Title) for o in
                                bsc(portal_type = 'AnalysisService',
                                    is_active = True)]

while your implementation uses item.UID:

items.append((item.UID, item.Title))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, changed it to use getKeyword.

templates_dir = resource_filename("bika.lims", instrpath)
fname = "%s/%s_import.pt" % (templates_dir, exim)
if exim.startswith('senaite/instruments'):
#TODO find a better way to see check if the instrument is in core
Copy link
Member

Choose a reason for hiding this comment

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

Yes, indeed. Not fancy to have senaite/instruments hard-coded here. But since (correct me if I am wrong), the plan is to move all these instrument interfaces to senaite.instruments, then we can consider this as a "temporary" thingy until all them get ported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I @xispa, I created a method to better determine is the instrument import interface is in or out of core. See latest commit

@xispa xispa added Cleanup 🧹 Code cleanup and refactoring PR: Rework-requested labels May 10, 2019
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.

Just added a comment re getAnalysisServicesDisplayList. The rest seems fine to me!

brains = bsc(portal_type='AnalysisService')
items = []
for item in brains:
items.append((item.UID, item.Title))
Copy link
Member

Choose a reason for hiding this comment

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

Hi @mikejmets , apologies for my slow reply. I've been out for two weeks.
Ummm... don't think so. Note the "ugly" previous implementation was using getObject().Keyword:

items = [('', '')] + [(o.getObject().Keyword, o.Title) for o in
                                bsc(portal_type = 'AnalysisService',
                                    is_active = True)]

while your implementation uses item.UID:

items.append((item.UID, item.Title))

@xispa xispa merged commit 096bf2f into senaite:master May 20, 2019
@mikejmets mikejmets deleted the find_adapted_interfaces branch May 21, 2019 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup 🧹 Code cleanup and refactoring
Development

Successfully merging this pull request may close these issues.

2 participants