List all multi-reports for samples, where the current sample is contained

[ramonski]

Description of the issue/feature this PR addresses

This PR refactors the published_results view for Samples to list also (multi-)reports, where the current sample is contained.

Current behavior before PR

Reports not displayed where the current sample was just contained

Desired behavior after PR is merged

All Reports displayed where the current sample is part of

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

[xispa]

I suggest to use the singular form contained_sample_uid instead. See https://github.com/senaite/senaite.core/blob/master/CODE_CONVENTIONS.md#plural-and-singular-forms

[xispa]

I would also simply name the index as sample_uid

[ramonski]

I suggest to use the singular form contained_sample_uid instead. See https://github.com/senaite/senaite.core/blob/master/CODE_CONVENTIONS.md#plural-and-singular-forms

Ah, sorry, forgot about that document. I will change it, but maybe rather to report_sample_uid to be more clear

[xispa]

I still think that is better to simply call it sample_uid. You are already filtering by portal_type, so is clear to me that you are searching for ARReports that have samples with the given uid contained.

Imagine you need the same thing for another type stored in same portal_catalog, but the samples are not contained in the "report", rather they are somehow just referenced in the catalogued object. If you keep report_sample_uid you don't have choice other than:

1. adding a new index (e.g. say referenced_sample_uid) or
2. re-use report_sample_uid

The first option adds unnecessary overhead (a new index) and the second option leads to confusion.

If you name the index as sample_uid, you can reuse the index without confusions. So, from my point of view, not adding "context"/"use-case" info in the index name gives us more flexibility, less confusion and less code.

[ramonski]

Hmm, really not sure if I'm missing here something, but since the index indexes an explicit attribute of ARReport, in this case getRawContainedAnalysisRequests I think it can not be extended, because it does not contain all Sample UIDs.
Therefore, I think naming it sample_uid is confusing, because it does not contain all the samples.

[xispa]

The assignment of ContainedAnalysisRequests takes place in senaite.impress here:
https://github.com/senaite/senaite.impress/blob/64f7c788ad4753a7262928aa04d5cef9c4683c41/src/senaite/impress/storage.py#L94

All samples (including the "primary") are passed through uids param to create_report function:
https://github.com/senaite/senaite.impress/blob/64f7c788ad4753a7262928aa04d5cef9c4683c41/src/senaite/impress/storage.py#L65

From what I see in the code above, I think all samples are considered, so the getRawContainedAnalysisRequests should always return sample uids this ARReport refers to/contains. If this is the case, then I think is safe to use sample_uid

Regarding the indexer, remember that you can have multiple indexers registered with same name, but for different content types and catalogs (I think yo need to declare them in different modules, but not a big deal imo). E.g:

@indexer(IARReport, IPortalCatalog)
def sample_uid(instance):
    return instance.getRawContainedAnalysisRequests()

@indexer(IBatch, IPortalCatalog)
def sample_uid(instance):
    return instance.getRawAnalysisRequests()

[ramonski]

I find that a bit confusing. The anatomy of the index I want to add is a 1..n relationship from one
Sample UID to many AR Reports.

From an index called sample_uid I would expect a 1..1 relationship from one Sample UID to one SampleObject and not to an AR Report.

[ramonski]

However, maybe my thinking is somehow limited by the UI Zope provides to add an index with the option to define the "Indexed attributes"

Or by the fact that it is not obvious if it is a 1..1 or 1..n relationship.

So in the code you added above, this index would also reference n batches and the portal_type query filter would either return the one or the other type (or both of course).

I'm going to change the naming now to sample_uid because actually you did recently more refactorings in the catalog indexes and invested more thinking in that topic...

[xispa]

Yes, I know that first impression might seem confusing. But all gets clear when you think about "how you search/sort" rather than "what the index contains". The index is an index, not metadata, so is meant to be used for searches and sortings only. In fact, you don't use the index to get the value it contains!. Therefore, it makes more sense to think about how you use it for searches and sorting when you define it's name.

For instance, if you have an index sample_uids (for ARReport) and you want to search the ARReports that contains a given sample. The search would look like this:

query = dict(
    portal_type="ARReport",
    sample_uids=uid   
)

At first glance seems ok, but doesn't the following make more natural to you?:

query = dict(
    portal_type="ARReport",
    sample_uid=uid   
)

And even if you want to search for ARReports that contain more than one sample, the second approach is still transparent (just search for any ARReport that contains any of the sample uids provided):

query = dict(
    portal_type="ARReport",
    sample_uid=uids   
)

Note that since this is a KeywordIndex, you can still search for those ARReports that don't have any Sample assigned (in this case it does not make sense, but for other indexes it might), just by storing an empty/None value in the index through the indexer:

@indexer(IARReport, IPortalCatalog)
def sample_uid(instance):
    return instance.getRawContainedAnalysisRequests() or [None]

You could then do a search for "missing" value (that sometimes is quite interesting):

query = dict(
    portal_type="ARReport",
    sample_uid=None   
)

In a nutshell. For me, plural form makes sense for metadata fields, because they are meant for data retrieval, but plural form does not make sense for index, because indexes are meant for searches/sorting.

Does this make sense to you?

[ramonski]

Thanks for your efforts explaining me! I think the major confusion is as you said, by mixing metadata and index behavior. Indeed, an index is just for finding/sorting and capable to filter out the results according to the given query. Therefore, yes, it makes more sense from the perspective of the query than from the perspective of the index architecture.

[xispa]

Thanks!