Refactored Remarks and created RemarksField and RemarksWidget #920

rockfruit · 2018-07-24T18:21:04Z

Description of the issue/feature this PR addresses

Remarks added during AR-Add (or perhaps other places) are modified with the widget's process_form, which can easily be mistakenly bypassed, resulting in entries without proper information being stored.

Current behavior before PR

Remarks entered in AR-Add form were saved without date/username header.

Desired behavior after PR is merged

Fields now use RemarksField and RemarksWidget.
Remarks entered in AR-Add or in portal_factory are stored correctly, Unless you do it on purpose, it should not be so easy to accidentally bypass the field's setter.
Where widget is rendered manually in custom templates, use View/Modify portal content permission check to decide if widget is displayed in edit/view mode.
Remarks field is editable in AR-Add XXX should this be on? I like them there, not sure if it's the right default.

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

ramonski

Please remove the changes that do not belong to this PR. PEP8 is ok, but changing code structures introduces too much potential for side-effects. Please also don't forget the time of the Reviewer looking at your code ...

In code try to not repeat yourself. If you find yourself writing multiple times the same structure, you should write a method/function for it.

The rest looks good, but please finish the requested changes first tomorrow in the morning with high priority. I really want these changes in as soon as possible;)

ramonski · 2018-07-25T20:39:33Z

bika/lims/browser/js/coffee/bika_widgets/remarkswidget.coffee

+     * Submit the value to the field setter via /@@API/update.
+     *
+    ###
+    widget = $(".ArchetypesRemarksWidget[data-uid='#{uid}']")


Don't repeat yourself;) Make a separate method for this and handle the case when the field is not found

ramonski · 2018-07-25T20:51:42Z

bika/lims/content/instrumentcertification.py

-            append_only=True,
-        ),
-    ),
+    RemarksField('Remarks',


Either put the name of the field in the next line (like the fields above) or put it in the same line as the opening brace. But not both styles mixed in one file please.

ramonski · 2018-07-25T20:52:53Z

bika/lims/content/instrumentcertification.py

-        else:
-            delta = valid_to - today
-
+        delta = valid_to - today


This actually does not belong to this PR

ramonski · 2018-07-25T20:53:33Z

bika/lims/content/invoice.py

        allowed_types=('AnalysisRequest',),
        relationship='AnalysisRequestInvoice',
    ),
    ReferenceField(
        'SupplyOrder',
        required=1,
-        vocabulary_display_path_bound=sys.maxsize,


Neither do these changes belong here. Actually I have no idea what these attributes are doing, why did you remove them?

ramonski · 2018-07-25T21:01:48Z

bika/lims/content/sample.py

@@ -828,7 +910,7 @@ def getLastARNumber(self):
        ar_ids = sorted([AR.id for AR in ARs if AR.id.startswith(prefix)])
        try:
            last_ar_number = int(ar_ids[-1].split("-R")[-1])
-        except:
+        except (IndexError, TypeError, ValueError):


This is actually nice, but you might introduce with that cosmetic change unexpected errors.
E.g. if ar_ids[-1] returns None you will get an AttributeError here.

Try to keep the focus more on the task to do please.

ramonski · 2018-07-25T21:04:54Z

bika/lims/content/worksheet.py

            return list()

        layout = self.getLayout()
        slots = list()

        for pos in layout:
-            if type != ALL_ANALYSES_TYPES and pos['type'] != type:
+            if typ != ALL_ANALYSES_TYPES and pos['type'] != typ:


You are introducing too much changes in this PR...

ramonski · 2018-07-25T21:08:35Z

bika/lims/content/worksheet.py

@@ -1535,12 +1546,11 @@ def copy_src_fields_to_dst(src, dst):
        new_ws_analyses = []
        old_ws_analyses = []
        for analysis in analyses:
+            position = analysis_positions[analysis.UID()]


nice catch, but does not belong to this PR. Please remove that and make a new one. Same for the type vs typ

ramonski · 2018-07-25T21:11:17Z

bika/lims/browser/js/coffee/bika_widgets/remarkswidget.coffee

+     *
+     * @returns {string} portal url
+    ###
+    return window.portal_url


Better use this method:

get_portal_url: => ### * Return the portal url (calculated in code) ### url = $("input[name=portal_url]").val() return url or window.portal_url

ramonski · 2018-07-25T21:12:14Z

bika/lims/browser/js/coffee/bika_widgets/remarkswidget.coffee

+
+    deferred = $.Deferred()
+    options =
+      url: window.portal_url + "/@@API/update"


use @get_portal_url() instead of window.portal_url please

Standard getter returns unmodified text.

ramonski

Puh, that one was harder than expected. Thanks Campbell!

This was because of senaite/senaite.core#920 Traceback (innermost last): Module ZPublisher.Publish, line 138, in publish Module ZPublisher.mapply, line 77, in mapply Module ZPublisher.Publish, line 48, in call_object Module Products.CMFPlone.FactoryTool, line 478, in __call__ Module ZPublisher.mapply, line 77, in mapply Module ZPublisher.Publish, line 48, in call_object Module Products.CMFFormController.FSControllerPageTemplate, line 91, in __call__ Module Products.CMFFormController.BaseControllerPageTemplate, line 32, in _call Module Shared.DC.Scripts.Bindings, line 322, in __call__ Module Shared.DC.Scripts.Bindings, line 359, in _bindAndExec Module Products.CMFCore.FSPageTemplate, line 237, in _exec Module Products.CMFCore.FSPageTemplate, line 177, in pt_render Module Products.PageTemplates.PageTemplate, line 87, in pt_render Module zope.pagetemplate.pagetemplate, line 132, in pt_render Module five.pt.engine, line 98, in __call__ Module z3c.pt.pagetemplate, line 158, in render Module chameleon.zpt.template, line 297, in render Module chameleon.template, line 191, in render Module chameleon.template, line 171, in render Module 338b44bf2274717b61b2323795a9c98f.py, line 1101, in render Module 338b44bf2274717b61b2323795a9c98f.py, line 911, in render_master Module 69e91da7ad509eec74265fa5c1df1777.py, line 1465, in render_master Module 69e91da7ad509eec74265fa5c1df1777.py, line 645, in render_content Module 338b44bf2274717b61b2323795a9c98f.py, line 901, in __fill_main Module 338b44bf2274717b61b2323795a9c98f.py, line 153, in render_main Module 5f0aa4601784b64af4844c8766340a88.py, line 414, in render_body Module Products.Archetypes.BaseObject, line 276, in widget Module Products.Archetypes.Renderer, line 26, in render Module Products.Archetypes.generator.widget, line 147, in __call__ AttributeError: Macro bika_widgets/remarks does not exist for <Patient at patient.2018-09-17.1793646232>

rockfruit requested a review from ramonski July 24, 2018 18:21

rockfruit added the Cleanup 🧹 Code cleanup and refactoring label Jul 24, 2018

ramonski added the PR: Not Ready ⛔️ label Jul 25, 2018

rockfruit removed the PR: Not Ready ⛔️ label Jul 25, 2018

ramonski requested changes Jul 25, 2018

View reviewed changes

ramonski added the PR: Rework-requested label Jul 25, 2018

rockfruit added 5 commits July 26, 2018 11:19

Add RemarksWidget

31b7351

Add RemarksField

71540c0

Fixup templates which manually render RemarksWidgets

22ffa7a

Remove remarks from some templates where it should not be shown

54418f4

Modify schemas to use RemarksWidget & RemarksField

1fb818c

rockfruit force-pushed the remarks branch from 192cced to 1fb818c Compare July 26, 2018 09:35

rockfruit and others added 7 commits July 26, 2018 11:35

Merge branch 'master' into remarks

a5c7922

Update changelog

4339613

Always display history box to allow setting remarks asynchronously

7962363

Some error handling in the JS logic

4dbe325

Handle traceback on remarks field update

879f894

ResultsField.get() returns HTML

9cedaae

Revert unrelated changes made in content sources

6446cc8

rockfruit removed PR: Rework-requested Cleanup 🧹 Code cleanup and refactoring labels Jul 26, 2018

rockfruit added 9 commits July 26, 2018 15:27

RemarksWidget: Add some classes to form elements

b7928fd

Remove label "history" from RemarksWidget

dc74513

Add remarks formatting directly to RemarksField.get

4709e18

Add remarks formatting directly to RemarksField.get (redux)

3022436

Prevent error in getter if RemarksField contains no value

27b1ed0

Remove misleading comment

ad04b5d

Remove authenticator from RemarksWidget template

dde5c1d

Add get_cooked_remarks to RemarksWidget...

4c8bf0a

Standard getter returns unmodified text.

Do not copy remarks when AR is copied to new AR.

d9c41da

Update template condition to decide if Remarks field is shown

ce3c4fe

ramonski approved these changes Jul 26, 2018

View reviewed changes

ramonski added Improvement 🔧 Cleanup 🧹 Code cleanup and refactoring labels Jul 26, 2018

ramonski merged commit 7fc0c8a into senaite:master Jul 26, 2018

ramonski mentioned this pull request Jul 26, 2018

Traceback on AR Template #927

Closed

ramonski added a commit that referenced this pull request Jul 27, 2018

Removed remarks.zcml as of #920

4cea630

ramonski mentioned this pull request Jul 27, 2018

Removed remarks.zcml #930

Merged

xispa pushed a commit that referenced this pull request Jul 27, 2018

Removed remarks.zcml as of #920 (#930)

72e5d53

xispa mentioned this pull request Sep 17, 2018

Fix Traceback "bika_widgets/remarks does not exist for ..." senaite/senaite.health#109

Merged

rockfruit deleted the remarks branch October 19, 2021 16:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored Remarks and created RemarksField and RemarksWidget #920

Refactored Remarks and created RemarksField and RemarksWidget #920

rockfruit commented Jul 24, 2018

ramonski left a comment

ramonski Jul 25, 2018

ramonski Jul 25, 2018 •

edited

Loading

ramonski Jul 25, 2018

ramonski Jul 25, 2018

ramonski Jul 25, 2018

ramonski Jul 25, 2018

ramonski Jul 25, 2018

ramonski Jul 25, 2018

ramonski Jul 25, 2018

ramonski left a comment

Refactored Remarks and created RemarksField and RemarksWidget #920

Refactored Remarks and created RemarksField and RemarksWidget #920

Conversation

rockfruit commented Jul 24, 2018

Description of the issue/feature this PR addresses

Current behavior before PR

Desired behavior after PR is merged

ramonski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramonski Jul 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramonski left a comment

Choose a reason for hiding this comment

ramonski Jul 25, 2018 •

edited

Loading