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

Opt out user office from missed reservation notifications #265

Open
wants to merge 120 commits into
base: master
Choose a base branch
from

Conversation

r-xyz
Copy link
Contributor

@r-xyz r-xyz commented Dec 20, 2024

Hi @rptmat57
I am opening this PR for an opt-out customization to remove User Office email from some notifications.

In first place I was thinking about removing it from the missed reservation notification. These emails already reach the abuse mailing list, same as "unauthorized tool access email" and "Out of time email"; the latter two notifications are not sent to user office, instead.
I was wondering if the discrepancy above was intended. If so, I would proceed to remove it through a Customization option, for the sake to avoid breaking changes.

I was also wondering if there are any similar cases which might make sense to include in the PR, either here or in NEMO-CE, to have the same approach.

On top of that, any other suggestion of different approaches is always welcome.

rptmat57 and others added 30 commits June 18, 2024 09:09
Extends functionality to accesses outside the allowed schedule for areas not requiring a reservation.
Relevant area -related changes are mirrored from [NEMO-CE Merge Request](https://gitlab.com/nemo-community/nemo-ce/-/merge_requests/31).
* Pickadate format now based on js format.
Closes usnistgov#237

* Pickadate format conversion: remove additional unsupported formats.

* Pickadate format conversion: fix submit format.

* Pickadate format: rename variable.

* Pickadate format: remove additional hardcoded value.

* Pickadate format: fix unsupported removal order.

* Pickadate format: fix typo.

* Pickadate format: fix typo.

* Pickadate format: remove duplicated formats.

* Pickadate format: improved logic.

Also renamed variables.

* Update NEMO/apps/kiosk/templates/kiosk/tool_reservation.html

* Update NEMO/templates/mobile/new_reservation.html

* fixed wrong format for date

---------

Co-authored-by: Mathieu Rampant <[email protected]>
…mplate (when someone forces a user off a tool but there were required questions to answer)
- added creation time to adjustment request admin page
…run data to "Start date" and to "End date" for post run data
- restricted impersonate so regular users cannot impersonate admins
rptmat57 and others added 27 commits November 4, 2024 10:09
- added "critical" flag to plugin apps to stop NEMO when there is an issue with loading a plugin (otherwise it's logged and ignored)
…isabling the same tool concurrently

- added preload_app = True to gunicorn
…ards

- for modbusTCP implementations, the "timeout" key/value can be specified as the timeout for the connection
* Improved required question star contrast.

* Improved required question highlighting.

Fixes usnistgov#256

* Moving invalid form input style to new `dynamic_form` class.

* Fixing required question line height.

* Port required form improvements to admin preview.

* Update NEMO/static/admin/dynamic_form_preview/dynamic_form_preview.css

---------

Co-authored-by: Mathieu Rampant <[email protected]>
…utocomplete for fields not linked to foreign keys
@rptmat57
Copy link
Contributor

that's a good idea. most of the notification emails being cc'd to one or the other email addresses are purely based on the CNST NanoFab requirements at the time.
It would be a good idea to make those more flexible.

I could see the "Email addresses" part of Customizations being extended to have checkboxes to allow facilities to decide which of the emails should they be included to. This could expand to the other email addresses, and potentially also regarding the emails being sent from those email addresses.

In short:

  1. Expanding Customization -> Email addresses to include checkboxes for emails they are cc'd on (not limited to User Office but this could be split into multiple PRs)
  2. Expanding it further to include checkboxes for emails being sent as them. If unchecked we would leave it blank which will use the default server email from settings.py (this could also be done in a separate PR)

How does that sound?

@r-xyz
Copy link
Contributor Author

r-xyz commented Dec 21, 2024

Thanks for the feedback.
It sounds good.
Would it be something like this, but for multiple notification types?
image

I see the other way around (having subsections for addresses instead of notification type) as more complicated to implement in UI, since you can only have one sender per notification.

@rptmat57
Copy link
Contributor

I am debating actually. I was originally thinking simply listing the emails sent from any of those addresses (only having choice between server email and that email) and whether they should be cced on stuff, but now I am rethinking this.

Here is what I am now thinking:

  1. adding more variable to the TemplatesCustomization class like this:
    def __init__(self, *args, **kwargs):
        for email_template, extension in self.files:
            self.__class__.variables[f"{email_template}_subject"] = ""
            self.__class__.variables[f"{email_template}_send_from"] = ""
            self.__class__.variables[f"{email_template}_cc_addresses"] = ""
        super().__init__(*args, **kwargs)
  1. the subject would be a django template string, allowing things like {{ variable }} following the same provided vars as the ones in the email template
  2. the send_from would have a datalist html widget with a few predefined choices (user_office, abuse, etc) and allow custom entry.
  3. the cc_addresses would allow multiple entries
  4. for #3 and #4 we could also simply use variable resolution to allow {{ user_office_email }} for example.
  5. for all the default values, I think the only way here would be to create a data migration that would add all the current send_from and subject and cc_addresses

This is a pretty big feature, and I probably haven't thought all of it through so we can discuss more, and if that's too much I also totally get it.

Let me know your thoughts.

Thanks,
Mathieu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants