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.

Sign up for GitHub

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

Jump to bottom

Traceback error when deleting attachment from Analysis Request #452

Merged

ramonski merged 3 commits into senaite:master from xispa:i441

Dec 7, 2017

Member

xispa commented Dec 7, 2017

Description of the issue/feature this PR addresses

The user cannot remove an attachment from an Analysis Request, but assigned to a particular Analysis. The reason is that while the relationship between AnalysisRequest and Attachment still uses ReferenceField, the relationship between Analysis and Attachment is based on an UIDReferenceField. The code from bika.lims.content.attachment that retrieves either the Analysis Request or the Analysis was always using the reference catalog. With this Pull Request, the retrieval of the Analysis the current Attachment is related to is done by using get_backreferences function from bika.lims.browser.uidreferencefield.

Linked issue: Traceback error when deleting attachment from AR #441

Current behavior before PR

A traceback is rised when trying to remove an Attachment from an Analysis Request that was explicitly assigned to an Analysis:

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 bika.lims.browser.attachment, line 81, in __call__
  Module bika.lims.browser.attachment, line 100, in action_update
  Module bika.lims.browser.attachment, line 189, in delete_attachment
AttributeError: 'NoneType' object has no attribute 'getAttachment'

Desired behavior after PR is merged

The user can remove an Attachment from an Analysis Request that was explicitly assigned to an Analysis.

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

xispa added 2 commits

December 7, 2017 11:16


          Make get_backreferences from UIDReferenceField more cleaner

1c6b7e3


          Cannot remove an Attaachment from Analysis Request

9f7153e

xispa added Bug 🐞 P1: Urgent labels

xispa requested a review from ramonski

December 7, 2017 10:28

ramonski requested changes

View reviewed changes

bika/lims/content/attachment.py Outdated

+                      if analysis_request:
+                          return getCurrentState(analysis_request)
+                      return ''

Contributor

ramonski Dec 7, 2017

Not sure about the possibilities to come to this point in code. Shall we add a logger.warn('Free floating attachment detected: {}'.format(repr(self)))?

Member Author

xispa Dec 7, 2017

I've removed getParentState function. Is not used anywhere in the code and seems quite awkward to me

bika/lims/content/attachment.py Outdated

+                          return None
+                      if len(analysis) > 1:
+                          logger.warn("Single attachment assigned to more than one Analysis")

Contributor

ramonski Dec 7, 2017

Just a question for better understanding: Why is it not allowed to have a single attachment attached to more than one Analysis? Wouldn't it make sense?

Member Author

xispa Dec 7, 2017

Nothing stops you to add more than one Attachment to the same Analysis, of course. Even the same file. What you cannot do is to associate the same Attachment object (with the same UID) to more than one Analysis. Every time you add an attachment, a new Attachment object will be created and associated to an Analysis, even if that Analysis already has other Attachment objects associated. Here we are using self(this particular instance of Attachment, with it's own UID) to get its back references, that atm, can only be one Analysis.
We could change and allow the user to associate multiple analyses to a single analysis in just one shot, of course. Then, the first thing we'd need to change is the attachments widget.

bika/lims/content/attachment.py Outdated

+                      return '%s - %s' % (request_id, analysis.Title())
                   def getRequest(self):
                       """Return the AR to which this is linked there is a short time between
                       creation and linking when it is not linked

Contributor

ramonski Dec 7, 2017

Ah indeed, here: https://github.com/senaite/bika.lims/blob/master/bika/lims/browser/analysisrequest/add2.py#L1800

bika/lims/content/attachment.py Outdated


		return '%s - %s' % (request_id, analysis.Title())

Contributor

ramonski Dec 7, 2017

reduce to single blank lines between methods.
Sorry, that might seem picky, but at least my flake8 linter is always complaining about these small issues...

bika/lims/content/attachment.py Outdated

-                              return requestid
-                      else:
+                      request_id = self.getRequestID()
+                      if not request_id:
                           return None

Contributor

ramonski Dec 7, 2017

Better return an empty string here, so it fit's better with the return type in line 149

bika/lims/content/attachment.py

+                      # from there.
+                      analysis = self.getAnalysis()
+                      if IRequestAnalysis.providedBy(analysis):
+                          return analysis.getRequest()

Contributor

ramonski Dec 7, 2017

Please be aware that you reach that point in code also if len(uids) != 1


          Small fixes and removal of getParentState function

a90a863

ramonski approved these changes

View reviewed changes

ramonski merged commit cf1f751 into senaite:master

xispa deleted the i441 branch

December 7, 2017 23:12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug 🐞 P1: Urgent