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

Apply category sort order on analysis specifications #2191

Open
wants to merge 7 commits into
base: 2.x
Choose a base branch
from

Conversation

anneline
Copy link
Contributor

Description of the issue/feature this PR addresses

Linked issue: https://github.com/senaite/senaite.core/issues/

Current behavior before PR

In analysis specification edit, analysis service categories were sorted alphabetically
97869461-c1c7-412f-8234-c47227b6b0a3

Desired behavior after PR is merged

In analysis specification edit, analysis service categories sorted by sort key

a95094b8-ab27-41e7-9540-91b62eb112b1

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

"""Sort by Categories
"""

bsc = getToolByName(self.context, "senaite_catalog_setup")
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use simply catalog as variable, because this catalog is no longer named bika_setup_catalog. You might also consider to use api.get_tool instead of getToolByName


if self.show_categories_enabled():
self.categories = map(lambda x: x[0],
sorted(self.categories, key=lambda x: x[1]))
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, above you have already all categories in the right order. It would make more sense to simply filter out those that are not found instead of sorting the found ones afterwards.
Use therefore the built in filter function of Python, e.g. like this:

from bika.lims import api
from senaite.core.catalog import SETUP_CATALOG

catalog = api.get_tool(SETUP_CATALOG)
all_categories = catalog({"portal_type": "AnalysisCategory", "sort_on: "sortable_title"})
sorted_categories = filter(lambda cat: api.get_title(cat) in self.found_categories, all_categories)
self.categories = map(api.get_object, sorted_categories)

Note: This code is not tested;)

if category not in self.categories:
self.categories.append(category)
if (category,cat_order) not in self.categories:
self.categories.append((category,cat_order))
Copy link
Contributor

Choose a reason for hiding this comment

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

Please simply collect here the category title in a instance variable, e.g.:

if category not in self.found_categories:
    self.found_categories.append(category)

This one is used later to filter out the already sorted categories.

@ramonski
Copy link
Contributor

Hi @anneline, what is the status of this PR?

@anneline
Copy link
Contributor Author

Updated as requested. Thanks Ramon.

@ramonski
Copy link
Contributor

Hi @anneline, did you commit the changes? I do not see any difference to the last time...

@ramonski
Copy link
Contributor

Please also ensure that the tests are passing, thanks!

@anneline
Copy link
Contributor Author

Done. Lint error removed.

@ramonski
Copy link
Contributor

Hi @anneline,

the requested changes are not here: https://github.com/senaite/senaite.core/pull/2191/files

Please change the code as requested in the comments above and commit/push the changes to this repository again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants