-
Notifications
You must be signed in to change notification settings - Fork 33
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 code for support #61
Conversation
zzacharo
commented
Oct 19, 2022
•
edited by kpsherva
Loading
edited by kpsherva
- address support form: integrate zenodo implementation #71
afe39fb
to
7eb327c
Compare
7eb327c
to
0f0c8ee
Compare
site/zenodo_rdm/assets/semantic-ui/js/zenodo_rdm/src/support/components/CategoryDropdown.js
Outdated
Show resolved
Hide resolved
site/zenodo_rdm/assets/semantic-ui/js/zenodo_rdm/src/support/components/SupportForm.js
Show resolved
Hide resolved
site/zenodo_rdm/assets/semantic-ui/js/zenodo_rdm/src/support/components/SupportForm.js
Show resolved
Hide resolved
...getInputProps(), | ||
// Display the dropzone input, otherwise it's hidden by default. | ||
style: { | ||
'display': 'block' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
react-dropzone
has an input
element that is hidden.
In other parts of invenio we keep it that way and re-implement it with either a button or a box for drag and drop. Here I went for a simpler solution, might not be the best though.
site/zenodo_rdm/assets/semantic-ui/js/zenodo_rdm/src/support/components/SupportForm.js
Outdated
Show resolved
Hide resolved
site/zenodo_rdm/assets/semantic-ui/js/zenodo_rdm/src/support/components/SupportForm.js
Show resolved
Hide resolved
site/zenodo_rdm/assets/semantic-ui/js/zenodo_rdm/src/support/components/SupportForm.js
Show resolved
Hide resolved
site/zenodo_rdm/assets/semantic-ui/js/zenodo_rdm/src/support/components/SupportForm.js
Outdated
Show resolved
Hide resolved
site/zenodo_rdm/assets/semantic-ui/js/zenodo_rdm/src/support/components/SupportForm.js
Outdated
Show resolved
Hide resolved
"""Constructor.""" | ||
super().__init__() | ||
|
||
def send_support_email( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be simplified into a method that works solely with contexts, as suggested by @slint personally.
I can tackle this in a separate task.
Same applies for the method send_confirmation_email
) | ||
|
||
|
||
class FormValidationError(ZenodoRDMError): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently not in use. Can be used when Marshmallow fails to validate the form data or simply removed (marshmallow already throws a ValidationError
) .
return redirect(url_for("invenio_app_rdm.index")) | ||
|
||
def handle_form(self, form_data): | ||
"""Form controller.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the only handler is to send two emails. However it can grow into different behaviors (e.g. based on a category do something else)
site/zenodo_rdm/support/support.py
Outdated
try: | ||
data = self.validate_form(data) | ||
except (ValidationError) as e: | ||
return str(e), 400 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unsure whether this error handling is ideal.
Please let me know if it can be improved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should raise (not return) a rest error, there are defined already in flask resources
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I realized 500 errors
already have a default handler and we can work with that.
For ValidationError
I added a custom handler. It returns an object similar to what we have in other modules, making the response serializable to formik. E.g.
{
"errors": [
{
"field": "email",
"messages": [
"Not a valid email address."
]
},
{
"field": "description",
"messages": [
"Description length must be bigger than 20 characters"
]
}
]
}
site/zenodo_rdm/assets/semantic-ui/js/zenodo_rdm/src/support/components/CategoryDropdown.js
Show resolved
Hide resolved
site/zenodo_rdm/assets/semantic-ui/js/zenodo_rdm/src/support/components/CategoryDropdown.js
Outdated
Show resolved
Hide resolved
site/zenodo_rdm/assets/semantic-ui/js/zenodo_rdm/src/support/components/FileTable.js
Outdated
Show resolved
Hide resolved
site/zenodo_rdm/assets/semantic-ui/js/zenodo_rdm/src/support/components/FileTable.js
Outdated
Show resolved
Hide resolved
site/zenodo_rdm/assets/semantic-ui/js/zenodo_rdm/src/support/components/FileTable.js
Outdated
Show resolved
Hide resolved
site/zenodo_rdm/assets/semantic-ui/js/zenodo_rdm/src/support/components/FileTable.js
Outdated
Show resolved
Hide resolved
const inpProps = { | ||
...getInputProps(), | ||
// Display the dropzone input, otherwise it's hidden by default. | ||
style: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this styling be moved to theme files?
site/zenodo_rdm/assets/semantic-ui/js/zenodo_rdm/src/support/components/SupportForm.js
Outdated
Show resolved
Hide resolved
site/zenodo_rdm/assets/semantic-ui/js/zenodo_rdm/src/support/components/SupportForm.js
Outdated
Show resolved
Hide resolved
site/zenodo_rdm/assets/semantic-ui/js/zenodo_rdm/src/support/components/SupportForm.js
Outdated
Show resolved
Hide resolved
site/zenodo_rdm/assets/semantic-ui/js/zenodo_rdm/src/support/components/SupportForm.js
Outdated
Show resolved
Hide resolved
const initialValues = { | ||
email: userMail, | ||
name: name, | ||
category: defaultCategory, | ||
sysInfo: false, | ||
files: null | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it make sense to be moved to the constructor instead of redeclaring it on each render?
"If this issue persists you can send " | ||
"us an email directly to {}".format(self.email_service.support_emails) | ||
) | ||
except (Exception) as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will catch any exception and will make debugging harder, is it on purpose ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it emulates the original one https://github.com/zenodo/zenodo/blob/master/zenodo/modules/support/views.py#L68, with more granular error catching (e.g. not sending the support email is not the same as not sending the confirmation email).
I agree that we lose the message and it will a nightmare to debug, especially since it is working with e-mail. Is there a standard way of logging exceptions in invenio?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all the comments can be addressed in the second iteration if that is easier to manage
0f0c8ee
to
6757fa4
Compare
site/zenodo_rdm/assets/semantic-ui/js/zenodo_rdm/src/support/components/CategoryDropdown.js
Outdated
Show resolved
Hide resolved
site/zenodo_rdm/assets/semantic-ui/js/zenodo_rdm/src/support/components/FileTable.js
Outdated
Show resolved
Hide resolved
site/zenodo_rdm/assets/semantic-ui/js/zenodo_rdm/src/support/components/SupportForm.js
Outdated
Show resolved
Hide resolved
site/zenodo_rdm/assets/semantic-ui/js/zenodo_rdm/src/support/components/SupportForm.js
Outdated
Show resolved
Hide resolved
site/zenodo_rdm/assets/semantic-ui/js/zenodo_rdm/src/support/components/SupportForm.js
Outdated
Show resolved
Hide resolved
site/zenodo_rdm/assets/semantic-ui/js/zenodo_rdm/src/support/components/SupportForm.js
Outdated
Show resolved
Hide resolved
site/zenodo_rdm/assets/semantic-ui/js/zenodo_rdm/src/support/components/SupportForm.js
Outdated
Show resolved
Hide resolved
site/zenodo_rdm/assets/semantic-ui/js/zenodo_rdm/src/support/components/FileTable.js
Outdated
Show resolved
Hide resolved
6757fa4
to
5a864aa
Compare
5a864aa
to
41910e4
Compare
41910e4
to
d118c1a
Compare