feat(contact): display Altinn Servicedesk contact if user belongs to org #14371
Conversation
github-actions
bot
added
solution/studio/designer
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14371 +/- ##
==========================================
- Coverage 95.66% 95.65% -0.02%
==========================================
Files 1880 1884 +4
Lines 24446 24473 +27
Branches 2810 2811 +1
==========================================
+ Hits 23386 23409 +23
- Misses 800 804 +4
Partials 260 260 ☔ View full report in Codecov by Sentry. |
framitdavid
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Co-authored-by: Nina Kylstad <[email protected]>
…tudio into feat/contactPage
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
frontend/packages/shared/src/getInTouch/providers/PhoneContactProvider.ts (1)
5-8: Consider moving phone numbers to configuration.While the implementation is clean, hardcoding phone numbers makes future updates more difficult and could lead to deployment issues if numbers need to change per environment.
Consider moving these to configuration:
-const phoneChannelMap: Record<PhoneChannel, string> = { - phone: 'tel:75006299', - emergencyPhone: 'tel:94490002', -}; +const phoneChannelMap: Record<PhoneChannel, string> = { + phone: `tel:${config.serviceDesk.phoneNumber}`, + emergencyPhone: `tel:${config.serviceDesk.emergencyPhoneNumber}`, +};frontend/studio-root/pages/hooks/queries/useFetchBelongsToOrgQuery.ts (1)
12-15: Consider adding error handling and caching configuration.While the query implementation is correct, it could benefit from additional configuration:
return useQuery({ queryKey: [QueryKey.BelongsToOrg], queryFn: () => fetchBelongsToGiteaOrg(), + staleTime: 5 * 60 * 1000, // Cache for 5 minutes + retry: false, // Don't retry on error + onError: (error) => { + // Handle error appropriately + console.error('Failed to fetch org membership:', error); + }, });backend/src/Designer/Helpers/AuthenticationHelper.cs (1)
27-30: Add XML documentation for the new method.While the implementation is correct, it's missing XML documentation to maintain consistency with other methods in the class.
+ /// <summary> + /// Checks if the current user is authenticated + /// </summary> + /// <param name="context">The HTTP context</param> + /// <returns>True if the user is authenticated, false otherwise</returns> public static bool IsAuthenticated(HttpContext context) { return context.User.Identity?.IsAuthenticated ?? false; }frontend/studio-root/components/ContactServiceDesk/ContactServiceDesk.tsx (2)
8-11: Move provider instantiation outside component.The contact providers are instantiated on every render. Consider moving them outside the component for better performance:
+ const contactByEmail = new GetInTouchWith(new EmailContactProvider()); + const contactByPhone = new GetInTouchWith(new PhoneContactProvider()); + export const ContactServiceDesk = (): ReactElement => { - const contactByEmail = new GetInTouchWith(new EmailContactProvider()); - const contactByPhone = new GetInTouchWith(new PhoneContactProvider()); return (
35-44: Remove unused phoneNumber value in email Trans component.The phoneNumber value is passed but not used in the email translation:
<Trans i18nKey='contact.serviceDesk.email' - values={{ phoneNumber: contactByPhone.url('phone') }} components={{ b: <b />, a: <StudioLink href={contactByEmail.url('serviceOwner')}>{null}</StudioLink>, }} />backend/tests/Designer.Tests/Controllers/ContactController/FetchBelongsToOrgTests.cs (1)
21-54: Add test coverage for missing scenarios.The tests cover basic authenticated and anonymous cases, but consider adding:
- Test for error handling when GetUserOrganizations throws
- Test for authenticated user without org membership
Example test case:
[Fact] public async Task BelongsToOrg_ShouldReturn_False_WhenGetUserOrganizationsThrows() { // Configure mock to throw var mockGitea = new Mock<IGitea>(); mockGitea.Setup(g => g.GetUserOrganizations()) .ThrowsAsync(new Exception("Test exception")); // Configure client with mock var client = Factory.WithWebHostBuilder(builder => { builder.ConfigureTestServices(services => { services.AddSingleton(mockGitea.Object); }); }).CreateDefaultClient(); string url = "/designer/api/contact/belongs-to-org"; using var httpRequestMessage = new HttpRequestMessage(HttpMethod.Get, url); var response = await client.SendAsync(httpRequestMessage); var responseContent = await response.Content.ReadAsAsync<BelongsToOrgDto>(); Assert.Equal(HttpStatusCode.InternalServerError, response.StatusCode); Assert.False(responseContent.BelongsToOrg); }backend/tests/Designer.Tests/Mocks/IGiteaMock.cs (1)
135-141: Consider using more descriptive mock data.While the mock data structure is correct, consider using more descriptive organization names that better represent real-world scenarios, such as "DigitalAgency" or "TechConsulting".
Apply this diff to make the mock data more descriptive:
var organizations = new List<Organization> { - new Organization { Username = "Org1", Id = 1 }, // Example items - new Organization { Username = "Org2", Id = 2 } + new Organization { Username = "DigitalAgency", Id = 1 }, + new Organization { Username = "TechConsulting", Id = 2 } };frontend/language/src/nb.json (1)
149-150: Consider enhancing accessibility of contact information.While the translations are well-structured, consider these improvements for better accessibility and user experience:
- Add ARIA labels for screen readers.
- Include additional context for the emergency phone number's availability hours.
- Consider adding tooltips or helper text explaining when to use each contact method.
Apply these changes to improve the translation strings:
"contact.altinn_servicedesk.content": "Er du tjenesteeier og har du behov for hjelp? Ta kontakt med oss!", "contact.altinn_servicedesk.heading": "Altinn Servicedesk", - "contact.serviceDesk.email": "<b>E-post:</b> <a>[email protected]</a>", - "contact.serviceDesk.emergencyPhone": "<b>Vakttelefon:</b> <a>94 49 00 02</a> (tilgjengelig kl. 15:45–07:00)", - "contact.serviceDesk.phone": "<b>Telefon:</b> <a>75 00 62 99</a>", + "contact.serviceDesk.email": "<b>E-post:</b> <a aria-label=\"Send e-post til [email protected]\">[email protected]</a>", + "contact.serviceDesk.emergencyPhone": "<b>Vakttelefon:</b> <a aria-label=\"Ring vakttelefon 94 49 00 02\">94 49 00 02</a> (tilgjengelig kl. 15:45–07:00, kun for kritiske hendelser)", + "contact.serviceDesk.phone": "<b>Telefon:</b> <a aria-label=\"Ring support på 75 00 62 99\">75 00 62 99</a> (hverdager kl. 08:00-15:45)"Also applies to: 156-158
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
backend/src/Designer/Controllers/ContactController.cs(1 hunks)backend/src/Designer/Helpers/AuthenticationHelper.cs(1 hunks)backend/src/Designer/Models/Dto/BelongsToOrg.cs(1 hunks)backend/tests/Designer.Tests/Controllers/ContactController/FetchBelongsToOrgTests.cs(1 hunks)backend/tests/Designer.Tests/Mocks/IGiteaMock.cs(2 hunks)frontend/language/src/nb.json(1 hunks)frontend/packages/shared/src/api/paths.js(1 hunks)frontend/packages/shared/src/api/queries.ts(2 hunks)frontend/packages/shared/src/getInTouch/providers/PhoneContactProvider.ts(1 hunks)frontend/packages/shared/src/mocks/queriesMock.ts(1 hunks)frontend/packages/shared/src/types/QueryKey.ts(1 hunks)frontend/studio-root/components/ContactSection/ContactSection.tsx(2 hunks)frontend/studio-root/components/ContactServiceDesk/ContactServiceDesk.tsx(1 hunks)frontend/studio-root/components/ContactServiceDesk/index.ts(1 hunks)frontend/studio-root/pages/Contact/ContactPage.test.tsx(2 hunks)frontend/studio-root/pages/Contact/ContactPage.tsx(4 hunks)frontend/studio-root/pages/hooks/queries/useFetchBelongsToOrgQuery.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/studio-root/components/ContactServiceDesk/index.ts
🧰 Additional context used
🪛 GitHub Check: CodeQL
backend/src/Designer/Controllers/ContactController.cs
[notice] 36-39: Generic catch clause
Generic catch clause.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: docker-build-push / Build and push docker image
- GitHub Check: Build environment and run e2e test
🔇 Additional comments (9)
backend/src/Designer/Models/Dto/BelongsToOrg.cs (1)
1-7: LGTM! Well-structured DTO implementation.The DTO is clean, follows C# conventions, and properly handles JSON serialization with camelCase naming.
frontend/packages/shared/src/getInTouch/providers/PhoneContactProvider.ts (1)
10-14: LGTM! Clean and type-safe implementation.The provider implementation is well-structured and leverages TypeScript's type system effectively.
frontend/studio-root/components/ContactSection/ContactSection.tsx (1)
Line range hint
9-13: LGTM! Well-implemented changes for conditional link rendering.The changes properly handle the optional link property, preventing runtime errors when the link is undefined. This aligns well with the PR's objective of conditional contact information display.
Also applies to: 34-34
backend/src/Designer/Controllers/ContactController.cs (1)
21-29: LGTM! Good use of AllowAnonymous attribute.The endpoint correctly handles unauthenticated users by returning a false response instead of a 401 status, which aligns with the PR objectives.
frontend/packages/shared/src/types/QueryKey.ts (1)
8-8: LGTM!The new query key
BelongsToOrgfollows the established naming convention and aligns with the PR objective.frontend/studio-root/pages/Contact/ContactPage.tsx (1)
20-22: LGTM!The changes are well-structured and follow best practices:
- The
ContactSectionMetadatatype is well-defined with an optional property.- The new contact section is properly integrated with conditional rendering.
- The filter function is pure and follows functional programming principles.
Also applies to: 69-75, 87-87, 96-98
frontend/packages/shared/src/api/queries.ts (1)
169-171: LGTM! Clean implementation of the organization membership check.The implementation follows the established patterns and is well-organized within the Contact Page section.
frontend/packages/shared/src/mocks/queriesMock.ts (1)
196-198: LGTM! Mock implementation is consistent with the codebase patterns.The mock function is well-placed and provides a reasonable default return value for testing.
frontend/packages/shared/src/api/paths.js (1)
187-189: LGTM! API path is well-structured and follows conventions.The endpoint path is correctly defined and properly organized within the Contact section.
|
Tested OK in dev |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/studio-root/pages/Contact/ContactPage.test.tsx (1)
55-78: Improve test descriptions for better clarity.While the test implementation is solid, consider making the test descriptions more descriptive:
- it('should not render contact info for "Altinn Servicedesk" if the user does not belong to a org', () => { + it('should hide Altinn Servicedesk contact info when user does not belong to any organization', () => { - it('should display contact information to "Altinn Servicedesk" if user belongs to an org', () => { + it('should display Altinn Servicedesk contact info when user belongs to an organization', () => {The improved descriptions:
- Better explain the test scenarios
- Make it clearer what behavior is being tested
- Follow the "when/then" pattern for better readability
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/tests/Designer.Tests/Controllers/ContactController/FetchBelongsToOrgTests.cs(1 hunks)frontend/language/src/nb.json(1 hunks)frontend/packages/shared/src/api/paths.js(1 hunks)frontend/packages/shared/src/api/queries.ts(2 hunks)frontend/packages/shared/src/mocks/queriesMock.ts(1 hunks)frontend/packages/shared/src/types/QueryKey.ts(1 hunks)frontend/studio-root/pages/Contact/ContactPage.test.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/packages/shared/src/mocks/queriesMock.ts
- frontend/packages/shared/src/api/paths.js
- frontend/packages/shared/src/api/queries.ts
- frontend/packages/shared/src/types/QueryKey.ts
- backend/tests/Designer.Tests/Controllers/ContactController/FetchBelongsToOrgTests.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build environment and run e2e test
🔇 Additional comments (2)
frontend/studio-root/pages/Contact/ContactPage.test.tsx (1)
5-11: LGTM! Mock setup is correctly implemented.The mock setup for
useFetchBelongsToOrgQueryis well-structured and follows testing best practices:
- Hook is properly imported and mocked
- Default mock return value is set to
{ data: { belongsToOrg: false } }frontend/language/src/nb.json (1)
156-157: LGTM! Translations are complete and well-structured.The Norwegian translations for Altinn Servicedesk contact info are properly added:
- Heading and content text for the contact section
- Contact details including email, phone, and emergency phone
- Translations follow consistent formatting with HTML tags
Also applies to: 163-165
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/tests/Designer.Tests/Controllers/ApiTests/TestAuthHandlerAnonymous.cs (1)
20-27: Add documentation to clarify the successful authentication behavior.While the implementation is correct, it might be counterintuitive that an anonymous user results in a successful authentication. Consider adding a documentation comment to explain this design decision.
+ /// <summary> + /// Simulates successful authentication of an anonymous user. + /// Returns Success to indicate the authentication process completed, + /// but with an empty claims identity to represent an unauthenticated user. + /// </summary> protected override Task<AuthenticateResult> HandleAuthenticateAsync()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/tests/Designer.Tests/Controllers/ApiTests/TestAuthHandlerAnonymous.cs(1 hunks)backend/tests/Designer.Tests/Controllers/ContactController/FetchBelongsToOrgTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/Designer.Tests/Controllers/ContactController/FetchBelongsToOrgTests.cs
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Run integration tests against actual gitea and db
- GitHub Check: Analyze
- GitHub Check: Format check
🔇 Additional comments (1)
backend/tests/Designer.Tests/Controllers/ApiTests/TestAuthHandlerAnonymous.cs (1)
1-19: LGTM! Well-structured authentication handler setup.The class follows best practices with proper dependency injection and clear naming that indicates its purpose as a test helper for anonymous authentication scenarios.
framitdavid
added
the
skip-manual-testing
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…tudio into feat/contactPage
Description
This PR introduces functionality to display Altinn Servicedesk contact information on the ContactPage. The contact details will only be visible to users associated with an organization.
Related Issue(s)
Verification
Testing the feature manually guide
Step 1: Test unauthenticated access
Step 2: Test access for authorized user
Step 3: Test access for unauthorized users
Summary by CodeRabbit
New Features
ContactServiceDeskcomponent with options for email and phone contact.Documentation
Tests
Chores