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

Solutions Feature Request Report #34026

Merged
merged 44 commits into from
Feb 9, 2024
Merged

Conversation

zandre-eng
Copy link
Contributor

@zandre-eng zandre-eng commented Jan 24, 2024

Product Description

A new dropdown option is added called "Solutions Feature Request":
Screenshot from 2024-01-24 15-20-30

Clicking on this button will cause a popup form to show:
Screenshot from 2024-01-24 15-20-43

After clicking Submit, a Jira ticket will be created in this project space.

It should be noted that this new Solutions feature request form is only available to staff members. Users that are not staff will not see the new dropdown option or the new modal form.

Technical Summary

Link to ticket here.

This PR adds a reporting system very similar to bug reports where a user can fill out a form which will automatically send an email. This email will, in turn, automatically create a Jira issue.

Note: This PR should only be merged once the changes from this PR have been merged. The linked PR will no longer be merged and the variable for the Solutions feedback email address will be inserted into CommCare HQ's code with this PR.

Feature Flag

None

Safety Assurance

Safety story

  • Local testing
  • Unit test
  • QA passed

Automated test coverage

Unit test exist for view sending email.

QA Plan

QA ticket has been created and is available here.

Note: QA has passed.

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@zandre-eng zandre-eng added Open for review: do not merge A work in progress product/all-users-all-environments Change impacts all users on all environments labels Jan 24, 2024
@dimagimon dimagimon added the Risk: Medium Change affects files that have been flagged as medium risk. label Jan 24, 2024
@zandre-eng zandre-eng marked this pull request as ready for review January 24, 2024 14:26
@zandre-eng zandre-eng requested a review from mkangia January 25, 2024 05:54
Copy link
Contributor

@mkangia mkangia left a comment

Choose a reason for hiding this comment

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

Hey Zandre,

Great PR, good flow of commits.
Left some blocking feedback to consider for improvements.

Comma-separated email addresses of others you want to notify about this request.
{% endblocktrans %}
<span class="badge bade-danger hide">
<i class="fa fa-warning"></i> {% trans 'Incorrect Format' %}
Copy link
Contributor

Choose a reason for hiding this comment

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

how is the format validated for emails?

Copy link
Contributor Author

@zandre-eng zandre-eng Jan 25, 2024

Choose a reason for hiding this comment

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

I have added the missing JS files (1b19cdf). The validation is happening in these files.

corehq/apps/hqwebapp/templates/hqwebapp/base.html Outdated Show resolved Hide resolved
corehq/apps/hqwebapp/views.py Outdated Show resolved Hide resolved
corehq/apps/hqwebapp/views.py Show resolved Hide resolved
@method_decorator([login_required], name='dispatch')
class SolutionsFeatureRequestView(View):
def post(self, request, *args, **kwargs):
email = _get_email_message_base(
Copy link
Contributor

Choose a reason for hiding this comment

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

lets also check here that the user is a staff member?

@@ -864,7 +864,7 @@ def post(self, request, *args, **kwargs):
post_params=request.POST,
couch_user=request.couch_user,
uploaded_file=request.FILES.get('feature_request'),
to_email=settings.SOLUTIONS_EMAIL,
to_email=settings.INTERNAL_FEEDBACK_EMAIL,
Copy link
Contributor

Choose a reason for hiding this comment

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

that might have been a better name actually.
dimagi/commcare-cloud#6211 (comment)

email.from_email = settings.SUPPORT_EMAIL

return email
email.send(fail_silently=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be good to tell the user all went well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After adding the missing JS files (1b19cdf), the user should now see a success message as follows:
Screenshot from 2024-01-25 12-23-39

to_email=settings.INTERNAL_FEEDBACK_EMAIL,
)
email.send(fail_silently=False)
if request.couch_user.is_staff:
Copy link
Contributor

Choose a reason for hiding this comment

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

should we return a 400 or similar in case the user isn't a staff?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, this would make it more obvious that something went wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -53,7 +54,7 @@ hqDefine('hqwebapp/js/bootstrap3/email-request', [
return false;
}

if (!self.isRequestReportSubmitting && self.$submitBtn.text() === self.$submitBtn.data("success-text")) {
if (!self.isRequestReportSubmitting && self.isReportSent) {
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

@mkangia
Copy link
Contributor

mkangia commented Jan 30, 2024

Thanks for the follow up @zandre-eng

Cool, will look forward to QA before I set this as approved. No blocking feedback now.

only available to Dimagi staff members

One small detail here would be to simply say "only available to Staff members" or "users marked as staff" or "users with staff access" to keep this generic.

@@ -36,7 +36,7 @@ hqDefine('hqwebapp/js/bootstrap5/email-request', [
self.$formElement.submit(() => {
resetErrors();

const isDescriptionEmpty = !self.subjectText() && !self.descriptionText();
const isDescriptionEmpty = !self.subjectText();
Copy link
Contributor

Choose a reason for hiding this comment

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

should the name be changed here as well? isSubjectEmpty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes that should. This should match how we have it for the bootstrap 3 version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zandre-eng zandre-eng added QA Passed and removed Open for review: do not merge A work in progress labels Feb 9, 2024
settings.py Outdated
@@ -491,6 +491,7 @@
DAILY_DEPLOY_EMAIL = None
EMAIL_SUBJECT_PREFIX = '[commcarehq] '
SAAS_REPORTING_EMAIL = None
INTERNAL_FEEDBACK_EMAIL = '[email protected]'
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need this now yes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't, I'll remove this (1d22c49)

Copy link
Contributor

@Charl1996 Charl1996 left a comment

Choose a reason for hiding this comment

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

Nice work, @zandre-eng !

@zandre-eng zandre-eng merged commit a75cf96 into master Feb 9, 2024
13 checks passed
@zandre-eng zandre-eng deleted the ze/solutions-feature-request-option branch February 9, 2024 12:31
@orangejenny
Copy link
Contributor

orangejenny commented Jun 11, 2024

FYI, this broke the report an issue functionality for B5 pages, and I don't think the solutions feature request modal has ever worked on B5 pages. There's logic around validating the subject that's missing from the B5 templates (see this diff line for some code that I suspect should be in both B3 and B5 versions of the template), which is causing validation to always fail.

@zandre-eng
Copy link
Contributor Author

FYI, this broke the report an issue functionality for B5 pages, and I don't think the solutions feature request modal has ever worked on B5 pages. There's logic around validating the subject that's missing from the B5 templates (see this diff line for some code that I suspect should be in both B3 and B5 versions of the template), which is causing validation to always fail.

@orangejenny Thanks for flagging this, I'll take a look into it. I think all the testing was done on B3 pages which is probably how this managed to slip past.

@orangejenny
Copy link
Contributor

@zandre-eng Cool, thank you. Yeah, I was looking at the date and figuring there might not have even been any B5 pages to test on at the time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/all-users-all-environments Change impacts all users on all environments QA Passed Risk: Medium Change affects files that have been flagged as medium risk.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants