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

Progress indicator for Batch listing #1544

Merged
merged 7 commits into from
Feb 17, 2020
Merged

Progress indicator for Batch listing #1544

merged 7 commits into from
Feb 17, 2020

Conversation

ramonski
Copy link
Contributor

@ramonski ramonski commented Feb 14, 2020

Description of the issue/feature this PR addresses

This PR adds progress indicators in batch listings, similar to those in Samples listing.
Furthermore, it is now possible to sort the column on progress.

Please merge senaite/senaite.app.listing#24 first

Current behavior before PR

  • No progress bar in Batch listing
  • Progress column not sortable

Desired behavior after PR is merged

  • Progress bar in Batch listing
  • Progress column sortable

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

@@ -2473,7 +2473,7 @@ def getProgress(self):
"""Returns the progress in percent of all analyses
"""
review_state = api.get_review_status(self)
if review_state == "published":
if review_state in ["published", "invalid"]:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shall we consider the cancelled state as well as complete?

Copy link
Member

@xispa xispa Feb 15, 2020

Choose a reason for hiding this comment

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

I wouldn't. "invalid" status can only be achieved after sample gets verified or published (so all analyses have been completed). "cancelled" happens in earlier stages and I think is ok to let the user know the percentage of conpleteness when the sample was cancelled, rather than forcing 100%

Copy link
Contributor

Choose a reason for hiding this comment

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

Please consider that progresses less than 100% indicate that some actions still have to be done, which is not the case for the cancelled samples.
In our case we cancel samples, when there is no more interest in them, so I would expect a 100% when all active samples are done.
I'm not sure what to do best with the "rejected" ones...

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 would agree with @grulisco to interpret all final states as 100% done. Reinstantiate e.g. cancelled Samples would anyhow correctly restore the progress to the actual state of the Analyses.

Copy link
Member

@xispa xispa Feb 16, 2020

Choose a reason for hiding this comment

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

If the criteria is that the progress indicates that no more actions are required (instead of the percentage of completed actions), then we should always apply a percentage of 100% to end-statuses, these being rejected, cancelled or published (invalid is a status that happens after publish/verified, so we always expect invalid to be 100% - we only added the status here to make the function a bit faster).

IMO, percentage of completeness fits better with the number of tasks/actions done over the number of expected tasks/actions: for me a 100% completed for a rejected or cancelled sample is not intuitive, but well, a matter of taste I guess. I'd say this confusion can only happen when you are listing Samples with filter "All" enabled (you'll never see rejected/cancelled samples when "Active"/"To be verified"/... filter is used).

Is ok to me if you feel is better the percentage to indicate if there are still actions to be done, but then I strongly suggest to apply same criteria for rejected.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say this confusion can only happen when you are listing Samples with filter "All" enabled (you'll never see rejected/cancelled samples when "Active"/"To be verified"/... filter is used).

Full ACK in samples listing, but we are listing batches here. So confusion will definitely happen.
This feature is useful as an overview on open batches only, because closed ones are always to be expected 100%.

I also agree with 100% for all end-status...

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 progress calculation in batches relies on the code from Sample, which means that both will behave consistently in the end. I'm going then to consider all final sample states as 100%, this will then make a batch 100% even when it contains cancelled/rejected/retracted samples next to some published ones.

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.

Works like a charm, thanks!

@xispa xispa merged commit 19c3e06 into master Feb 17, 2020
@xispa xispa deleted the progress-in-batchlisting branch February 17, 2020 23:43
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.

3 participants