EES-5469 - Add feedback submission banner and admin view #5494
Conversation
tomjonesdev
requested review from
benoutram,
bennettstuart,
mmyoungman and
jack-hive
src/GovUk.Education.ExploreEducationStatistics.Admin/Controllers/Api/Bau/FeedbackController.cs
Outdated
Show resolved
Hide resolved
src/GovUk.Education.ExploreEducationStatistics.Admin/Controllers/Api/Bau/FeedbackController.cs
Outdated
Show resolved
Hide resolved
src/explore-education-statistics-frontend/src/components/Feedback.tsx
Outdated
Show resolved
Hide resolved
ntsim
changed the title
|
|
||
| public DateTime Created { get; set; } | ||
|
|
||
| public string Url { get; set; } = string.Empty; |
There was a problem hiding this comment.
Could make this required so we don't have to set this to an empty string.
Same can be done for Response property below
|
|
||
| public DateTime Created { get; set; } | ||
|
|
||
| public string Url { get; set; } = string.Empty; |
There was a problem hiding this comment.
Can make this required to avoid needing the empty string default
|
|
||
| namespace GovUk.Education.ExploreEducationStatistics.Content.Api.Requests; | ||
|
|
||
| public record CreateFeedbackRequest |
There was a problem hiding this comment.
Would name this FeedbackCreateRequest to be consistent with normal naming conventions where the more significant nouns come first. This result in better ordering in the file structure.
| {feedbackItems && ( | ||
| <tbody> | ||
| {feedbackItems ? ( |
There was a problem hiding this comment.
Multiple checks on feedbackItems is unnecessary. They're also incorrect as we're setting an empty array as the default.
Additionally, the empty state displaying the table with a single row isn't the correct pattern. We should do something like this instead:
{feedbackItems.length > 0 ? (
<table>...</table>
} : (
<InsetText>No feedback found</InsetText>
)}| <input type="hidden" name="userAgent" value={userAgent} /> | ||
| <input type="hidden" name="url" value={url} /> | ||
| <input type="hidden" name="response" value={response} /> |
There was a problem hiding this comment.
Would remove these as it's simpler to add these to the directly to the request on form submit.
| Yes <VisuallyHidden>this page is useful</VisuallyHidden> | ||
| </Button> | ||
|
|
||
| <Button |
There was a problem hiding this comment.
We're missing aria-expanded and aria-controls attributes that are needed so screen readers can inform the user that the form section has been opened.
Same applies for the 'Report' button below as well.
|
|
||
| public string? Issue { get; set; } | ||
|
|
||
| public string? Intent { get; set; } |
There was a problem hiding this comment.
Looks like we're missing server-side validation - we should add FluentValidation rules that should at least match the database constraints.
| public class FeedbackController(ContentDbContext context) : ControllerBase | ||
| { | ||
| [HttpPost] | ||
| public async Task<ActionResult> CreateFeedback( |
There was a problem hiding this comment.
Would be good to get some integration tests in here. We could also use these to test that the server-side validations (mentioned in another comment) are working correctly.
| [Route("api")] | ||
| [ApiController] | ||
| [Authorize(Roles = RoleNames.BauUser)] | ||
| public class FeedbackController(ContentDbContext context) : ControllerBase |
There was a problem hiding this comment.
Could use some integration tests for this?
| intent?: string; | ||
| } | ||
|
|
||
| export default function Feedback() { |
There was a problem hiding this comment.
Need tests for this, particularly as it's user facing!
tomjonesdev
force-pushed
the
EES-5469
branch
2 times, most recently
from
ed30a90 to
f35b221
Compare
|
|
||
| Assert.Equivalent(feedback, result); | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing a test for filtering using ShowRead?
| namespace GovUk.Education.ExploreEducationStatistics.Admin.Migrations.ContentMigrations | ||
| { | ||
| /// <inheritdoc /> | ||
| public partial class EES5469_IncreaseFeedbackColumnLimits : Migration |
There was a problem hiding this comment.
Given that we're this is a new data model that hasn't been introduced to the main branches yet, it'd be better to merge this migration with the prior one (i.e. a single migration).
We already have a tonne of migrations piling up so it'd be preferable to keep the migration history as clean as possible to make re-baselining these in the future easier.
| var saved = await context.Feedback.FirstAsync(); | ||
|
|
||
| Assert.Equivalent(request, saved); | ||
| } |
There was a problem hiding this comment.
Would be good to have some tests that check the validation's working correctly as well.
| <table> | ||
| <tbody> | ||
| <tr> | ||
| <th scope="row">Date</th> | ||
| <td> | ||
| <FormattedDate format="d MMM yyyy, HH:mm"> | ||
| {feedback.created} | ||
| </FormattedDate> | ||
| </td> | ||
| </tr> | ||
| <tr> | ||
| <th scope="row">URL</th> | ||
| <td>{feedback.url}</td> | ||
| </tr> | ||
| <tr> | ||
| <th scope="row">Response</th> | ||
| <td>{getResponseText(feedback.response)}</td> | ||
| </tr> | ||
| {feedback.response !== 'Useful' && ( | ||
| <> | ||
| <tr> | ||
| <th scope="row" className="dfe-white-space--nowrap"> | ||
| What were you doing? | ||
| </th> | ||
| <td>{feedback.context ?? '-'}</td> | ||
| </tr> | ||
| <tr> | ||
| <th scope="row" className="dfe-white-space--nowrap"> | ||
| What went wrong? | ||
| </th> | ||
| <td>{feedback.issue ?? '-'}</td> | ||
| </tr> | ||
| <tr> | ||
| <th scope="row" className="dfe-white-space--nowrap"> | ||
| What did you hope to achieve? | ||
| </th> | ||
| <td>{feedback.intent ?? '-'}</td> | ||
| </tr> | ||
| </> | ||
| )} | ||
| <tr> | ||
| <th scope="row">User agent</th> | ||
| <td>{feedback.userAgent}</td> | ||
| </tr> | ||
| </tbody> | ||
| </table> |
There was a problem hiding this comment.
Would use SummaryList component instead. This is the correct component for data that is formatted like this (i.e. where there is a 'key' and 'value' per row).
See: https://design-system.service.gov.uk/components/summary-list/
| feedback: FeedbackViewModel; | ||
| getResponseText: ( | ||
| response: FeedbackViewModel['response'], | ||
| ) => 'Useful' | 'Not useful' | 'Problem encountered' | ''; |
There was a problem hiding this comment.
Don't need to specify the exact string literals as we're not doing any logic that requires this level of type safety.
A type of string is sufficient and will make it easier to change the labels in the future.
| const getResponseText = (response: FeedbackViewModel['response']) => { | ||
| switch (response) { | ||
| case 'Useful': | ||
| return 'Useful'; | ||
| case 'NotUseful': | ||
| return 'Not useful'; | ||
| case 'ProblemEncountered': | ||
| return 'Problem encountered'; | ||
| default: | ||
| return ''; | ||
| } | ||
| }; |
There was a problem hiding this comment.
This can be hoisted to module-level as it's a pure function with only inputs and outputs.
We can also export this so it can just be used in the FeedbackDetailsModal directly. This way we don't need to pass this function into the component as a prop and can just import it into the component.
| var feedback = await context.Feedback | ||
| .OrderByDescending(x => x.Created) | ||
| .Select(f => MapToViewModel(f)) | ||
| .ToListAsync(cancellationToken); | ||
|
|
||
| return Ok(feedback); |
There was a problem hiding this comment.
I'm not personally a big fan of this as it'll become quickly apparent the page will need it. Pagination is also super easy to implement as we've got all the primitives to do so i.e. Paginate extension method for IQueryable and the Pagination component in common.
However, happy for you to make the final call on this.
| </td> | ||
| <td> | ||
| <ButtonText | ||
| className="dfe-white-space--nowrap govuk-!-margin-bottom-3" |
There was a problem hiding this comment.
Nitpick - could tighten this up a little bit more to 2 spacing units
| setBannerState('thanks'); | ||
| }; | ||
|
|
||
| const feedbackLimit = 2000; |
There was a problem hiding this comment.
Might want to hoist this to module level as it doesn't change
| return Yup.object({ | ||
| context: Yup.string().max( | ||
| feedbackLimit, | ||
| `Description must be ${feedbackLimit} characters or less`, |
There was a problem hiding this comment.
Error messages should be uniquely identifiable in the error summary. Currently, if we have max length errors in both fields we end up with the following:
It's unclear which error corresponds to which field, which is confusing if you try and click on one of the error links.
Would suggest we change this to 'What were you doing must be 2000 characters or less'.
A similar thing can be done for the other messages below:
- What were you hoping to achieve must be 2000 characters or less
- What went wrong must be 2000 characters or less
| {response && | ||
| (bannerState === 'notUseful' || | ||
| bannerState === 'problemEncountered') && ( |
There was a problem hiding this comment.
To ensure that screen readers know what element is being opened by the button (with aria-controls), we should make sure to have the element in the DOM before the button is pressed.
Currently, we're conditionally rendering the form so it won't be present in the DOM before the button is pressed and screen readers may miss the association.
We should replace the use of conditional rendering with the HTML hidden attribute so that the form is in the DOM, but is not visible to screen readers (or users) until the button is pressed:
<div
id="feedbackFormContainer"
className="..."
hidden={bannerState === 'notUseful' || bannerState === 'problemEncountered')}
>Note we can omit response as it's not necessary in the conditional.
Also note that to change to this, we may need to use toBeVisible for various test assertions (instead of toBeInTheDocument).
| <Button | ||
| className={styles.buttonYesNo} | ||
| variant="secondary" | ||
| ariaControls="feedbackForm" |
There was a problem hiding this comment.
Wrong id to use - should use feedbackFormContainer instead
|
|
||
| describe('Feedback', () => { | ||
| test('renders initial state correctly', () => { | ||
| render(<Feedback />); |
There was a problem hiding this comment.
Could add a line break after the render method to make it more obvious where rendering ends and assertions begin. Currently, there isn't a lot of breathing room and makes it less readable.
Would typically suggest leaving line breaks between any significant 'stages' of a test (i.e. arrange, act and assert stages) to improve readability.
|
|
||
| test('renders correctly and submits when "Yes" is selected', async () => { | ||
| render(<Feedback />); | ||
| await userEvent.click( |
There was a problem hiding this comment.
Although you'll currently see many places in the code where we're using the userEvent module directly, this is no longer the correct approach as Testing Library was updated in v14 with a new API that looks like:
const user = userEvent.setup();
render(<Feedback />);
await user.click(...);See: https://testing-library.com/docs/user-event/intro/#writing-tests-with-userevent
At least for new code we should be using the new API. For older usages of userEvent, we should try remember to update these to the new API when we encounter them.
Similar can be done below in rest of test case as well.
| ); | ||
|
|
||
| const contextTextArea = screen.getByLabelText('What were you doing?', { | ||
| selector: 'textarea', |
There was a problem hiding this comment.
Selector isn't necessary here. It's only needed for specific cases where the input is not possible to get using just the label alone.
Same in all cases below as well.
| test('renders correctly and submits when "Yes" is selected', async () => { | ||
| render(<Feedback />); | ||
| await userEvent.click( | ||
| screen.getByRole('button', { name: 'Yes this page is useful' }), |
There was a problem hiding this comment.
We should check that the text isn't visible before the button is pressed as well to check that a change actually occurred
| expect(screen.getByLabelText('What were you doing?')).toBeInTheDocument(); | ||
| expect( | ||
| screen.getByLabelText('What were you hoping to achieve?'), | ||
| ).toBeInTheDocument(); |
There was a problem hiding this comment.
We should check that these aren't visible before the button is pressed as well to check that a change actually occurred
Same applies for equivalent 'Report a problem' test case below as well
This PR includes two pieces of work:
The first part allows users to quickly and easily share feedback on specific areas of the site, and the second provides a simple UI to view this feedback in the "Platform administration" area. This work is to help ensure the product owner and publishers are better informed of user interactions in order to aid decision making, and consider suggestions which may include changes to the platform or publication content.
NB. The administrator page appearance and functionality was a small piece of additional work added for convenience, so there were no designs or specs - I'm happy to consider any suggestions which may improve the UI or UX of this page. Also I've not included pagination, this can be added at a later date if required.
Banner
Admin area