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

Migrate Calculations to DX #2600

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

Conversation

toropok
Copy link
Contributor

@toropok toropok commented Jul 29, 2024

Description of the issue/feature this PR addresses

This PR migrates the AT Calculation to Dexterity for Python 3 compatibility.

Current behavior before PR

Calculation is a content type based on the Archetypes framework

Desired behavior after PR is merged

Calculation is a content type based on the Dexterity framework

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

@toropok toropok marked this pull request as ready for review November 18, 2024 14:11
@toropok
Copy link
Contributor Author

toropok commented Nov 18, 2024

Hi @ramonski @xispa,

Here's my preliminary delivery for the calc migration! 😊 This migration turned out to be quite a tough nut compared to the previous ones—no chance it would’ve worked without the invaluable help of @Bugerman58! 🚀

Currently, we’re stuck at a point where the tests are failing. Specifically:

We’re not using HistoryAwareReferenceField as expected.
Was there any thought about using an alternative?
Or is there no longer a need to maintain version references?
Looking forward to your input!

@ramonski
Copy link
Contributor

ramonski commented Nov 19, 2024

Hi Leonid, hi Anton,
many thanks for your continuous efforts and support that we really appreciate!
Yes, indeed, you picked a tough nut with the calculations 🤯
Regarding the HistoryAwareReferenceWidget, Jordi and I discussed once that we want to drop it and maybe lookup the values from the Snapshot storage? And even combine this with our UIDReferenceField?
Anyhow, we'll find a solution for this for sure.
Thanks again for your good work and we'll review as soon as possible!

@xispa
Copy link
Member

xispa commented Nov 20, 2024

Excellent work @toropok and @Bugerman58 🥇 . I can imagine how challenging the migration of Calculation has been!. Your amazing work is really appreciated! Willing to review the Pull Request already and maybe we open another one later for the removal of the HistoryAware relic!

@xispa xispa added Cleanup 🧹 Code cleanup and refactoring Content Migration ♻️ Migrate contents to Dexterity labels Nov 20, 2024
@xispa xispa self-requested a review November 26, 2024 08:51
@xispa
Copy link
Member

xispa commented Nov 26, 2024

Hi @toropok , tests are failing https://github.com/senaite/senaite.core/actions/runs/11948903328/job/33307294669

Can you please take a look before I start to review?

@toropok
Copy link
Contributor Author

toropok commented Nov 26, 2024

Hi @toropok , tests are failing https://github.com/senaite/senaite.core/actions/runs/11948903328/job/33307294669

Can you please take a look before I start to review?

Hi @xispa

Sure, I'll fix the failed Python tests (they only test object validation) and get back to you. The remaining failing tests are doctests, which are related to HistoryAwareReferenceField.

@toropok
Copy link
Contributor Author

toropok commented Dec 2, 2024

@xispa

Sorry for the long pause.

  • I’ve added FormulaValidator, and it works! 🙂
  • Fixed the Python tests — all tests are now passing successfully.

That leaves us with two remaining issues that need your guidance and vision:

  1. HistoryAware behavior for UIDReferenceField: All the failing doctests are related to this feature.
  2. Formula wildcards: How are they supposed to work? It seems like they’ve never worked as intended.

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.

Pheew, a lot of work here... @toropok you must be exhausted. I've done a first review and requested some changes though.

Haven't gone through the validators details yet, but I think you can simplify a lot there. I feel are overwhelming, will review them in the next round.

I've seen tests that are failing are because of the HistoryAwareReferenceField. Don't worry too much about this for now.

src/senaite/core/api/analysisservice.py Show resolved Hide resolved
src/senaite/core/upgrade/v02_06_000.py Outdated Show resolved Hide resolved
src/senaite/core/browser/form/adapters/calculation.py Outdated Show resolved Hide resolved
src/senaite/core/upgrade/v02_06_000.py Outdated Show resolved Hide resolved
None).validate(
{"formula": deep_get(data, "form", FIELD_FORMULA) or "",
"interims":
[{"keyword": i} for i in self.get_interim_keywords(data)]})
Copy link
Member

Choose a reason for hiding this comment

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

Really nice, so system "detects" if an interim field or service for a given keyword does exist each time a form value is edited.... definitely this helps a lot!!

However, would you mind to simplify the code a bit? something like below would be easier to read imho:

form = data.get("form")

validator = FormulaValidator(self.context, self.request, self, ICalculationSchema, None)
value = {
    "formula": form.get(FIELD_FORMULA) or "",
    "interims": self.get_interim_fields(data)
}
errors = validator.validate(value)

Note I would rely on a self.get_interim_fields(data) that returns well-formed interim field records, containing not only with the keyword, but also the rest of subfields (you never know the subfields the validator will rely on).

src/senaite/core/browser/viewlets/configure.zcml Outdated Show resolved Hide resolved
auto_append=True)
interims = InterimsField(
title=_(u"label_calculation_interims",
default=u"Calculation Interim Fields"),
Copy link
Member

Choose a reason for hiding this comment

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

Please use "Variables" for title and description, like we have for AnalysisService: https://github.com/senaite/senaite.core/blob/2.x/src/bika/lims/content/analysisservice.py#L112-L119

I think is cleaner. From a user perspective, "interim" sounds confusing imo.

src/senaite/core/content/calculation.py Show resolved Hide resolved
src/senaite/core/workflow/calculation/guards.py Outdated Show resolved Hide resolved
@xispa
Copy link
Member

xispa commented Dec 11, 2024

  1. HistoryAware behavior for UIDReferenceField: All the failing doctests are related to this feature.

Yes, I've seen. I've requested some changes, we will handle this HistoryAware behavior afterwards.

  1. Formula wildcards: How are they supposed to work? It seems like they’ve never worked as intended.

They are supposed to work, yes. For instance, if the user types this formula:

[CHOL] + [FAT] if [CHOL] >= [CHOL.LDL] else "Invalid"

where:

  • CHOL is the keyword of a service (e.g. Cholesterol)
  • FAT is the keyword of a saervice (e.g. Fat)
  • <service_keyword>.LDL is the wildcard representing the value of the field LowerDetectionLimit for the prefixed service

Therefore, if the result of CHOL is below the LowerDetectionLimit, the result of the calculation will be Invalid.

See e.g. the doctests here: https://github.com/senaite/senaite.core/blob/2.x/src/senaite/core/tests/test_calculations.py#L83-L137

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup 🧹 Code cleanup and refactoring Content Migration ♻️ Migrate contents to Dexterity
Development

Successfully merging this pull request may close these issues.

3 participants