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 custom action provider for direct PDF sharing via email #132

Merged
merged 25 commits into from
Feb 7, 2023

Conversation

ramonski
Copy link
Contributor

@ramonski ramonski commented Feb 4, 2023

Description of the issue/feature this PR addresses

This PR adds a custom action provider based on #131, that allows to send the generated PDF directly via Email:

The action button is not a

ctive by default and can be enabled in the IMPRESS controlpanel:

Current behavior before PR

It is not possible to send a generated PDF directly w/o storing it first, e.g. for custom templates meant for receipt confirmation.

Desired behavior after PR is merged

It is possible to send a generated PDF directly w/o storing it first, e.g. for custom templates meant for receipt confirmation.

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

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.

Just minor things/questions regarding code.

Regarding functionality,

  • would be possible to hide the model after the "Send" button is clicked. I feel users will keep pushing the button and multiple emails will be sent otherwise. Alternatively, display a message and disable the "Send" button
  • I get a TypeError: this is undefined when I either click the "Email" or "Save" buttons now
  • I feel we need to somehow make clear that whilst "Email" button transitions the sample to published status, the rest of actions don't. Maybe an impress setting?

src/senaite/impress/actions/providers.py Show resolved Hide resolved
src/senaite/impress/actions/providers.py Show resolved Hide resolved
self.context = context
self.request = request
# see senaite.impress.interfaces.ICustomFormProvider
self.available = False
Copy link
Member

Choose a reason for hiding this comment

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

Since in most cases, the "availability" of the action provider will depend on some logic, I think it would be better if available was a function instead of an attribute. I feel the constructor should only be in charge to initialize the basics and not call internal functions if possible. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good point. I'll change it (also in the interface description)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented here: 2e45c0e

@ramonski
Copy link
Contributor Author

ramonski commented Feb 6, 2023

  • would be possible to hide the model after the "Send" button is clicked. I feel users will keep pushing the button and multiple emails will be sent otherwise. Alternatively, display a message and disable the "Send" button

It is kept open to show the Status Message:

SENAITE IMPRESS 2023-02-06 1 PM-02-59

However, this was not displayed in FireFox and fixed here:

# NOTE: type is different per browser, e.g.:
# -> FireFox: "text/html; charset=utf-8"
# -> Chrome: "text/html"
type = blob.type

But in general, closing the modal after submit can be configured in the action adapter with the attribute close_after_submit:

class ICustomActionProvider(Interface):
    """Provide additional action buttons in report preview
    """
    available = Attribute(
        "Boolean to control if the action should be rendered")
    title = Attribute(
        "Action title that will be rendered for the button")
    text = Attribute(
        "Help text that should be displayed inside the button")
    name = Attribute(
        "Uniqe button name of the action")
    url = Attribute(
        "Form action or modal view URL")
    modal = Attribute(
        "Boolean to control if the action should open a new modal")
    close_after_submit = Attribute(
        "Boolean to control if the modal should be closed after submit")
    css_class = Attribute(
        "CSS class of the action button")

    def get_action_data():
        """Returns the known attributes to the ReactJS component
        """

@ramonski
Copy link
Contributor Author

ramonski commented Feb 6, 2023

  • I get a TypeError: this is undefined when I either click the "Email" or "Save" buttons now

Should be fixed in 6e556d2

@ramonski
Copy link
Contributor Author

ramonski commented Feb 6, 2023

Just minor things/questions regarding code.

Regarding functionality,

  • would be possible to hide the model after the "Send" button is clicked. I feel users will keep pushing the button and multiple emails will be sent otherwise. Alternatively, display a message and disable the "Send" button
  • I get a TypeError: this is undefined when I either click the "Email" or "Save" buttons now
  • I feel we need to somehow make clear that whilst "Email" button transitions the sample to published status, the rest of actions don't. Maybe an impress setting?

I thought so as well. What do you think about the following approach:

  • We remove the Save and the Email button and make it just Store which defaults to the Email View. In the setting we have a configuration which allows to disable the default redirect to the Email view, so that you end up with the behavior we have with "Save" at the moment

@xispa
Copy link
Member

xispa commented Feb 7, 2023

  • I feel we need to somehow make clear that whilst "Email" button transitions the sample to published status, the rest of actions don't. Maybe an impress setting?

I thought so as well. What do you think about the following approach:

* We remove the `Save` and the `Email` button and make it just `Store` which defaults to the Email View. In the setting we have a configuration which allows to disable the default redirect to the Email view, so that you end up with the behavior we have with "Save" at the moment

Not sure, it might be confusing. Even if user wants the system to behave as it does with Email button most of the time is still useful to have the Save button as is. Maybe the solution is to make the actions and button texts to be more explicit:

  • Download: downloads the generated PDF
  • Share: shares the generated PDF (now by email but maybe through another channel in future via modal)
  • Save: saves the generated PDF
  • Publish: saves the generated PDF and publishes the sample
  • Share and publish: saves, shares the generated PDF and publishes the sample

We could even be less confusing by removing Share and publish and simply prompt the user with a confirmation panel like "Do you want to share the results report" [Yes/No] when the "Publish" button is clicked. If user clicks to Yes, system redirects to the email view we have. Otherwise, saves the report and transitions the sample. So this would then look as follows:

  • Download: downloads the generated PDF
  • Share: shares the generated PDF (now by email but maybe through another channel in future via modal)
  • Save: saves the generated PDF
  • Publish: saves the generated PDF, publishes the sample and prompts the user to share the results report

However, to be coherent I think we should then rename the transition "Publish" by "Results report preview" or something alike. So the user would see an action with this name that clearly states that it will be redirected to the results report preview, from where one would be able to do several actions, but not necessarily publish.

@ramonski
Copy link
Contributor Author

ramonski commented Feb 7, 2023

Thanks for your input!
However, we would need then to change the "Publish" behavior, because the samples are not published directly after the report is created. This first happens after the Email was sent, or the user clicked on a PDF report and clicks again the (Force-)"Publish" Button.
Let's do this in another PR, there are some more in the queue on my side...

@xispa
Copy link
Member

xispa commented Feb 7, 2023

Thanks for your input! However, we would need then to change the "Publish" behavior, because the samples are not published directly after the report is created. This first happens after the Email was sent, or the user clicked on a PDF report and clicks again the (Force-)"Publish" Button. Let's do this in another PR, there are some more in the queue on my side...

👍

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.

Excellent and exciting, keep pushing!

@xispa xispa merged commit e246126 into 2.x Feb 7, 2023
@xispa xispa deleted the hookable-send-email-action branch February 7, 2023 09:25
@xispa xispa mentioned this pull request Feb 7, 2023
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