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

Issue-455: Hiding DatePublishedViewer on the AR View and populating #456

Closed

Conversation

Lunga001
Copy link
Contributor

@Lunga001 Lunga001 commented Dec 8, 2017

DatePublished on the AR View

Description of the issue/feature this PR addresses

Linked issue: #455

Current behavior before PR

 Date Published appears two times

Desired behavior after PR is merged

  Date Published should appear once

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

@@ -1470,7 +1470,7 @@
description=_("The date when the request was published"),
visible={
'edit': 'visible',
'view': 'visible',
'view': 'invisible',
Copy link
Member

Choose a reason for hiding this comment

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

DatePublishedViewer field can be totally removed. Was added in #159 because at that moment, there was no DatePublished field, but a getDatePublished function that retrieves the date from the publish transition timestamp: https://github.com/senaite/bika.lims/blob/master/bika/lims/content/analysisrequest.py#L2601-L2605

During the merge in October, the field DatePublished was added without taking into account that DatePublishedViewer field was already there. On the other hand, the name of DatePublish field fits better and is already tagged as read-only. I suggest to remove DatePublishedViewer, but make DatePublished a ComputedField, so the value will always be stored in the schema and we will not need to apply workarounds such as the one above in header_table

attr = "get{}".format(fieldname)
if hasattr(self.context, attr):
attr = getattr(self.context, attr)
value = attr()
Copy link
Member

Choose a reason for hiding this comment

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

The call to the function should have priority over the value stored in the Schema's field. Also, if there is a Schema's field, we can assume a getter by default, so I think the following would be better:

elif field.getType().lower().find('datetime') > -1:
    attr = "get{}".format(fieldname)
    value = getattr(self.context, attr)()

Nevertheless, please read the comment below re DatePublishedViewer and DatePublished fields, cause I think this attr workaround will not be needed.

@Lunga001
Copy link
Contributor Author

Made the requested changes but there's no test at the moment

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.

Please, remind to add expression parameter when using ComputedField and never forget backwards-compatibility via upgrade steps.

@@ -1431,7 +1431,7 @@
),
),

DateTimeField(
ComputedField(
Copy link
Member

Choose a reason for hiding this comment

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

There is no expression parameter for this ComputedField, so I am sure it will not work. Consider that there is a function getDatePublished that might be helpful on this regard:
https://github.com/senaite/senaite.core/blob/master/bika/lims/content/analysisrequest.py#L2599-L2605

Take also into account that changing the type of a field from a Schema usually requires to apply the change to all objects from this type that are already stored in the database. This is done by adding the proper code in an upgradestep. This procedure is required to guarantee backwards-compatibility.

@@ -32,6 +32,7 @@ Changelog

**Changed**

- #455 On the AR View Date Published should appear once
Copy link
Member

Choose a reason for hiding this comment

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

Please, updated this branch to the latest master and move this log from "1.1.8 (2017-12-23)" to "1.2.1 (unreleased)" section.

@Lunga001
Copy link
Contributor Author

Hi @xispa I've add the changes and used clearAndRebuild on the upgrade script and was wondering if portal_catalog,
bika_catalog_analysisrequest_listing,
bika_analysis_catalog are the only catalogs that that I have to run the clearAndRebuild on

@xispa
Copy link
Member

xispa commented Jan 18, 2018

@Lunga001 no please, never run a cleanAndRebuildin upgrade steps, from a performance prespective is a bad idea: doing a cleanAndRebuild to an instance with say, 50k ARs/analyses may take hours... imagine the time it would take for an instance with 100k Analysis, > 5h (if RAM does not get exhausted before the upgrade finishes). In upgrade steps always try to be as much precise as possible, to only update those objects that require the upgrade and only the fields that have been changed. The same with reindex, always do reindexObject for specific objects, never do a reindex for the whole catalog!.

@Lunga001
Copy link
Contributor Author

Hi @xispa after having a chat with @mikejmets it seems that an upgrade script is not needed because even though the field type has been changed(from DateTimeField to ComputedField), the field is still using the same index(getDatePublished) and this index does not change.

@xispa
Copy link
Member

xispa commented Jul 18, 2018

Closing this one in favor of #904 , cause there is no way (permissions problem) to push to lunga's branch

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.

2 participants