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

Accessibility #153

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Accessibility #153

wants to merge 3 commits into from

Conversation

fapdash
Copy link
Contributor

@fapdash fapdash commented Mar 22, 2022

I plan to do this for all the icon only buttons.
I also added the test setup in this PR so I could write a test for my changes.

We could also add `.secure(true)` to all `MockMvc` calls.
We would need to add the https config to the
application-test.properties then.
Since it makes our tests marginally faster and otherwise
doesn't cost anything I prefer this solution.
This helps screen readers.
Users on desktop can also hover over the icon to
get a textual description of what the button does.

@Test
void profileView__validUser__profileDisplayed() throws Exception {
Mockito.when(authService.getCurrentUser()).thenReturn(testUsers.get(0));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bit silly as we really don't need to mock the AuthService once we actually have working auth in tests.
If we don't mock the AuthService then the ApplicationContext can't be reused though.
But I think the @AutoConfigureMockMvc will change the ApplicationContext hash anyways, since it adds the MockMvc to the context.
My suggestion would be to move all the @SpringBootTest classes to this pattern.

@Nonononoki
Copy link
Contributor

For constants please use the Tools.java class. If you need to check for the profile there is an example is DonateService:125, no need for ProfileConstants class.

@fapdash
Copy link
Contributor Author

fapdash commented Mar 23, 2022

If you need to check for the profile there is an example is DonateService:125, no need for ProfileConstants class.

If you insist on doing it that way that's fine but the way you coded this we can never run multiple profiles at the same time since your check for the production profile will fail in that case.
That's why I recommend against it. Using the Environment class let's the Spring Framework handle the profile matching.

@Nonononoki
Copy link
Contributor

What's the use case for running multiple profiles at the same time?
Your solution is indeed the more elegant one, but now it must be changed everywhere else as well ;)

@fapdash
Copy link
Contributor Author

fapdash commented Mar 23, 2022

What's the use case for running multiple profiles at the same time?

I use it to quickly mix in predefined behavior during development, which I don't always want to have turned on.
Eg. extensive SQL logging or dumping the schema generated by hibernate.ddl-auto to a file (the sql DDL statements).
I also use it in production but that's mainly because I prefer property files over environment variables.
In general I think it's a good idea to keep as many options open as possible because it gets harder to change such decisions the bigger the code base grows. 🤷 :)

Your solution is indeed the more elegant one, but now it must be changed everywhere else as well ;)

I might go ahead and do it to get more familiar with the code base but I'm not sure when I'll find the time. 🤷

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants