-
Notifications
You must be signed in to change notification settings - Fork 492
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
As a researcher, I want to know what dataset/dataverse a contact email is referring to so that I can follow up #1916
Comments
Great suggestion @suenjedt we should do what we do for notifications where we include links to the dataset and dataverses they are referencing. |
This part is mentioned in #3545. |
#2882 seems related as well. |
@mheppler @scolapasta @dlmurphy and anyone else who's interested in what text appears in the "bundle", I think I'm mostly done messing with the text as of 6c22b9e if you'd like to take a look at the "diff" in Bundle.properties at https://github.com/IQSS/dataverse/pull/4571/files Please note that since I've been focused on providing more context for dataverses, datasets, and files, I haven't thought much about adding more context to the generic "Support" emails but if someone wants to suggest something I can probably work it in without too much trouble since it's the same code. I'm staying on this issue to do some testing to make sure I didn't break anything with all my refactoring to make the code testable. Ok, also, @qqmyers popped in IRC this morning and he's going to take a look at related issue #4580. I explained that we're working on the content first but then we'll want to address how in some cases these emails aren't being delivered, which is what that other issue is about. |
Thanks, @pdurbin. Bundle text looks good in the PR diff! For the support emails, I think we can go a little more generic. I've cut down the template to something a little more streamline since we don't need to pass any dataverse, dataset, file info.
|
The bundle text looks good to me, thanks! Also, Mike's change to the support emails looks good to me too. |
Bah. The content is better but the code itself is buggy and needs work. I'm pounding on it. |
I don't understand the difference betweet subField.getDisplayValue() and subField.getValue() but the former was null for email addresses so I'm switching to the latter for both the email and and the name of the contact. Also get API tests passing.
Ok, as of 197c57d I think the pull request is in pretty good shape for code review. We could keep doing more on this branch, including:
I'll keep myself on this issue for now but developers are welcome to give me feedback on the code. |
In 038d5a3 I went ahead and added more context to the support emails, with minor tweaks approved by @mheppler @mheppler and I are on the same page that at this time I am not going to pursue adding a field for "full name" or whatever we call it. The main reason for this is that I'd have to touch a lot of files and this isn't a high priority. Here's a screnshot of the files we'd need to modify: In addition to the files above the change would need to be made in If or when we add "full name" we should decide how to label it. Perhaps we could write "Your Name" and "Your Email" like this: The second benefit of having the second field is that we'll be able populate Since I think I'm done hacking on the code (barring any bugs I introduced), I'm taking myself off this issue and I'm ready for code review. |
Reviewed, moved into QA. |
Tested new context email works as described with the exception of |
@kcondon sorry, I believe we have a misunderstanding. It does work that way, or it should. Support emails are sent from the email address provided in the support form. That's the "from" address. So replying to that from address should reach the individual who provided an email address in the support form. Of course, if you're logged in, when you fill the the support form, you shouldn't be asked for an email address. The email on file for your account will be used. I hope this all makes sense! |
Updated support email template text to remove inaccurate reply instructions.
The issue for Contact Us Email: Set the reply to address to be from the user, from address from dataverse. Otherwise email blocked. #4580 should address existing issues with the reply address. I will add a comment on that issue as well. There will need to be consideration for how our RT system works when we change the from and reply fields in the support emails. |
I tried to contact dataset contacts and received the emails as expected. I just wanted to raise that there was no context info given: which dataset/dataverse, who wrote (I was logged in) etc. Thought that might be helpful for potential recipients.
The text was updated successfully, but these errors were encountered: