Skip to content

Footer #871

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

Merged
merged 32 commits into from
Feb 22, 2023
Merged

Footer #871

merged 32 commits into from
Feb 22, 2023

Conversation

bjosttveit
Copy link
Member

@bjosttveit bjosttveit commented Jan 27, 2023

Description

Removed the proposed grid-solution as I think it could be too complicated for a user friendly configuration in studio. Currently just a simple flexbox. Cypress tests depend on Altinn/app-lib-dotnet#183 being released and frontend-test updating nugets.

Related Issue(s)

Verification

  • Manual testing
    • I have tested these changes manually
    • Creator of the original issue (or service owner) has been contacted for manual testing
    • No testing done/necessary
  • Automated tests
    • Unit test(s) have been added
    • Cypress E2E test(s) have been added
    • No automatic tests are needed here
    • I want someone to help me make some tests
  • User documentation @ altinn-studio-docs
    • No changes/updates needed
  • Changes/additions to component properties
    • Changes are reflected in both src/layout/layout.d.ts and layout.schema.v1.json, and these are all backwards-compatible
    • No changes made
  • Support in Altinn Studio
    • Issue(s) created for support in Studio
  • Sprint board
    • The original issue (or this PR itself) has been added to the Team Apps project and to the current sprint board

@bjosttveit bjosttveit added the kind/product-feature Pull requests containing new features label Jan 27, 2023
@bjosttveit bjosttveit changed the title WCAG: Added footer with accessibility statement and contact info Footer Feb 2, 2023
@ivarne
Copy link
Member

ivarne commented Feb 6, 2023

Sorry for not noticing this earlier, but I somehow feel this is very related to the issues about customizing the header.

Why did you go with a custom footer.json file instead of making this just a footer property of applicationsettings.json or ul/../Settings.json?

Another consequence of the current setup is that we need yet another http call to show an app (making loading even slower), and for all older apps it will fill our logs with 404 Not Found {org}/{app}/api/footer, both in console and in application insights (where you might be able to configure an ignore rule.)

@bjosttveit
Copy link
Member Author

Those issues are indeed related, and are something we also want to allow customization of.

I suppose the reason we did not want to define it in applicationMetadata.json is that it would not be very flexible in case we wanted to allow different footers depending on the process step in the future. It cannot really be put into Settings.json since the footer needs to be available outside of the process, like instance selection. Yes, it would cause 404 in older apps as of now. 😕

@bjosttveit
Copy link
Member Author

We have discussed this again today, and we have decided to keep it in a separate file with the current setup for now. And then at a later point we can combine multiple files into a single request were appropriate to reduce the number of requests when loading the form.

@bjosttveit bjosttveit marked this pull request as ready for review February 7, 2023 09:03
@ivarne
Copy link
Member

ivarne commented Feb 7, 2023

OK: Looking forward to Application Insights being full of 404 errors looking for a footer.
image

@bjosttveit bjosttveit requested a review from Magnusrm February 7, 2023 09:54
@olemartinorg
Copy link
Contributor

OK: Looking forward to Application Insights being full of 404 errors looking for a footer.

@ivarne Same thing when looking for api/v1/featureset in #899 soon. We'll discuss this on our end, to try to find a solution. IMO, 404 is barely an "error" (reminder that 4xx means "you did something wrong" while 5xx is "I did something wrong", and if lots of 4xx responses on an app trigger all the alarm bells it would be very easy for any malicious actor to make those alarm bells go off).

@ivarne
Copy link
Member

ivarne commented Feb 7, 2023

@olemartinorg Yes, but api/v1/featureset is one endpoint that will reduce noise in the future. api/v1/footer is yet another minor feature that shouldn't need to probe for the existence of an api.

Copy link
Contributor

@olemartinorg olemartinorg left a comment

Choose a reason for hiding this comment

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

Looks good! 🚀

I think we should have automatic tests for this thing (as I believe all features should have some form of automatic tests 😉). You can even use Cypress to mock the API response to test out different footers. I used that approach in #899, which made it a joy to develop the functionality (as I could have Cypress open in a different window where it tests the entire feature I'm in the process of making, while I'm making it - it caused me to find and fix several edge-cases). Without tests, anyone can break this functionality without noticing.

@bjosttveit
Copy link
Member Author

Nice, I assumed I needed to update frontend-test with a footer and updated nugets to test with cypress. If it is possible to mock I can do it before merging 👍

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

50.9% 50.9% Coverage
0.0% 0.0% Duplication

@bjosttveit bjosttveit merged commit 58ddfbb into main Feb 22, 2023
@bjosttveit bjosttveit deleted the footer branch February 22, 2023 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/product-feature Pull requests containing new features
Projects
None yet
3 participants