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

Add COPY button on AST panel and AST breakpoint #27

Merged

Conversation

nnpvaan-infostatus
Copy link
Contributor

@nnpvaan-infostatus nnpvaan-infostatus commented Jul 19, 2023

Description of the issue/feature this PR addresses

Add COPY button in AST breakpoint table page and AST panel page

Linked issue: https://github.com/beyondessential/pnghealth.lims/issues/288

Current behavior before PR

AST breakpoint and AST panel page don't have the "Duplicate" button

Desired behavior after PR is merged

When the "Duplicate" button is clicked, the system displays a new view where the user can set the values Title

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

@nnpvaan-infostatus nnpvaan-infostatus force-pushed the ast-panels-and-breakpoints-duplicate branch from 11afacd to 3376e76 Compare July 19, 2023 09:44
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.

Thanks @nnpvaan-infostatus , I have left some comments. Please, simplify the _call_ function from DuplicateView

src/senaite/ast/utils.py Outdated Show resolved Hide resolved
src/senaite/ast/view.py Outdated Show resolved Hide resolved
src/senaite/ast/view.py Outdated Show resolved Hide resolved
src/senaite/ast/view.py Outdated Show resolved Hide resolved
src/senaite/ast/view.py Outdated Show resolved Hide resolved

<metal:content-core fill-slot="content-core">
<span class="text-danger" tal:condition="python:not view.objects" i18n:translate="">
No AST Breakpoint were selected.
Copy link
Member

Choose a reason for hiding this comment

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

Better No AST Breakpoints tables were selected

class="text-muted text-secondary"
i18n:translate=""
tal:condition="python:view.objects">
Enter the details of each of the AST Breakpoint you want to copy.
Copy link
Member

Choose a reason for hiding this comment

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

Enter the details for each AST Breakpoints table you want to copy

class="text-muted text-secondary"
i18n:translate=""
tal:condition="python:view.objects">
Enter the details of each of the AST Panel you want to copy.
Copy link
Member

Choose a reason for hiding this comment

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

Enter the details for each AST panel you want to copy


<metal:content-core fill-slot="content-core">
<span class="text-danger" tal:condition="python:not view.objects" i18n:translate="">
No AST Panel were selected.
Copy link
Member

Choose a reason for hiding this comment

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

No AST panels were selected

<table class="table table-borderless table-hover">
<tr class="border-bottom">
<th>AST Panel</th>
<th>Title</th>
Copy link
Member

Choose a reason for hiding this comment

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

Missing i18n:translate attribute:

<th i18n:translate="">AST Panel</th>
<th i18n:translate="">Title</th>

<table class="table table-borderless table-hover">
<tr class="border-bottom">
<th>AST Breakpoint</th>
<th>Title</th>
Copy link
Member

Choose a reason for hiding this comment

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

Missing i18n:translate attribute:

<th i18n:translate="">AST Breakpoints table</th>
<th i18n:translate="">Title</th>

"""Checks if the given title is valid and unique. Returns an
error message if not valid. None otherwise
"""
import pdb;pdb.set_trace()
Copy link
Member

Choose a reason for hiding this comment

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

Remove this pdb

titles = self.request.form.get("title", [])
self.created = []
for i, s in enumerate(sources):
if not titles[i]:
Copy link
Member

Choose a reason for hiding this comment

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

This if not titles[i] condition can be moved to the logic where you check if the title exists, inside copy_object function, so the whole loop becomes simpler:

self.savepoint = savepoint()
self.created = []
uids = self.request.form.get("uids", [])
titles = self.request.form.get("title", [])
for index, uid in enumerate(uids):
    title = titles[index]
    obj = self.copy_object(uid, title)
    if not obj:
        self.savepoint.rollback()
        self.created = []
        break

    self.created.append(title)

Note that if the system cannot create an object (e.g. because the title exists or no title was set), the whole transaction is rolled back, so all objects created previously are dismissed. This guarantees that objects are only created if all them have "valid" information.

return

# Create a copy
return api.copy_object(source, title=title, ShortTitle="")
Copy link
Member

Choose a reason for hiding this comment

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

You can remove ShortTitle, cause this field belongs to Analysis-like objects only:

return api.copy_object(source, title=title)

"""Creates a copy of the given object, but with the given title
"""
# Validate the title
portal_type = get_portal_type(get_object(source))
Copy link
Member

Choose a reason for hiding this comment

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

Better use api explicitely, like api.get_portal_type and api.get_object

@xispa xispa merged commit bf6fb73 into senaite:master Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants