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

PR 2269 - Show the Unit in Manage Analyses View #281

Merged
merged 10 commits into from
Oct 24, 2017
Merged

PR 2269 - Show the Unit in Manage Analyses View #281

merged 10 commits into from
Oct 24, 2017

Conversation

juangallostra
Copy link
Contributor

@juangallostra juangallostra commented Oct 23, 2017

Linked issue: bikalims/bika.lims#2268

Behavior before PR

  • Unit is not displayed in the "Manage Analyses" view

Desired behavior after PR is merged

  • Unit column is displayed per default

Comments

I just noticed that on the Analysis Requests view the Priority column has no header text (see image attached). I think it might be better to name it for clarity.

screenshot from 2017-10-24 10-54-19

Copy link
Contributor

@rockfruit rockfruit left a comment

Choose a reason for hiding this comment

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

It seems the priority svg files are missing, the priorities are not displayed when I test with this PR

items[x]["max"] = spec.get("max",'')
items[x]["error"] = spec.get("error",'')
items[x]['Price'] = analysis.getPrice()
analysis.getService().getKeyword())
Copy link
Contributor

@rockfruit rockfruit Oct 23, 2017

Choose a reason for hiding this comment

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

This should be analysis.getKeyword(). In senaite.lims/master, all attributes of the originating service are stored directly on the analysis. In any event, if you do find you want to get the service, this attribute is now called analysis.getAnalysisService()

Copy link
Member

Choose a reason for hiding this comment

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

Agree, use analysis.getKeyword() instead of analysis.getService().getKeyword()... note analysis.getService() is flagged as deprecated

item["error"] = spec.get("error", '')
# Add priority premium
item['Price'] = analysis.getPrice()
item['Priority'] = analysis.getPriority()
Copy link
Contributor

@rockfruit rockfruit Oct 23, 2017

Choose a reason for hiding this comment

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

Analysis.getPriority() no longer exists; these were those awful AT objects, which have been replaced by AnalysisRequest.getPrioritySortkey. It also exists as Analysis.getPrioritySortkey(), which inherits the priority of it's parent AR.

Copy link
Member

@xispa xispa Oct 23, 2017

Choose a reason for hiding this comment

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

Yes, agree with your appreciation @rockfruit. Anyhow, I think that since this is the "manage_analyses" view from inside AR and the priority assigned to an Analysis corresponds tot that one assigned to the AR to which the analysis belongs, I don't think "Priority" column here being required. My suggestion is not to add this column.

if service is not None:
uid = api.get_uid(service)
if uid:
rr_dict_by_service_uid[uid] = r
Copy link
Member

Choose a reason for hiding this comment

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

In this case, better to not use get_service_by_keyword here. IMO, the previous implementation was better, cause it was not waking-up objects, rather using brains. I'd wouldn't replace any logic here.

items[x]["max"] = spec.get("max",'')
items[x]["error"] = spec.get("error",'')
items[x]['Price'] = analysis.getPrice()
analysis.getService().getKeyword())
Copy link
Member

Choose a reason for hiding this comment

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

Agree, use analysis.getKeyword() instead of analysis.getService().getKeyword()... note analysis.getService() is flagged as deprecated

@xispa
Copy link
Member

xispa commented Oct 24, 2017

@rockfruit the Priority icon was not rendered because item[Priority][replace]) must be used and something like the following chunk added there:
https://github.com/senaite/bika.lims/blob/senaite-integration/bika/lims/browser/analysisrequest/analysisrequests.py#L829-L832

Anyhow, priority column in this list has been removed, cause it doesn't make any sense (the getPriority or getPrioritySortKey from an Analysis delegates to its parent AR). If we add priority at analysis level (regardless of AR's priority) in future, then of course, I think we'll need to display the column.

@xispa xispa dismissed rockfruit’s stale review October 24, 2017 21:43

Since the Priority column has been removed, there is no need to display priority icons here

@xispa xispa merged commit 49392bc into senaite:senaite-integration Oct 24, 2017
@rockfruit rockfruit mentioned this pull request Oct 24, 2017
37 tasks
@juangallostra juangallostra deleted the 2269 branch March 22, 2018 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants