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.

Sign up for GitHub

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

Jump to bottom

Field of law improvements #2342

Merged
merged 64 commits into from
Nov 27, 2024
Merged

Field of law improvements #2342

merged 64 commits into from
Nov 27, 2024

Conversation

elaydis
Copy link
Contributor

@elaydis elaydis commented Nov 6, 2024

RISDEV-5342

@elaydis elaydis added the dev-env Automatically spins up developer environment label Nov 6, 2024
elaydis and others added 23 commits November 6, 2024 12:00
@elaydis
Trigger Pipeline
80a5aea 
RISDEV-0000
@leonie-koch
Combine direct input and search input
fa95455 
RISDEV-5347
@leonie-koch
Fix frontend tests
29da7c5 
RISDEV-0000
@elaydis
Comment out broken tests
dd8b8ea 
RISDEV-0000
@elaydis
Use query with wildcard and ignore case
5859bfe 
RISDEV-5347
@elaydis
Split search three ways
164758c 
RISDEV-5347
@leonie-koch
Add loading spinner to fields of law search
eb80376 
RISDEV-5349
@elaydis
Layout adjustments
d25eeff 
RISDEV-5344
@elaydis
Comment out failing test
f27b153 
RISDEV-5344
@elaydis
Fix style
cf51b33 
RISDEV-5344
@elaydis
Skip failing tests
d8e8227 
RISDEV-0000
@elaydis
Comment out failing tests
9388af0 
RISDEV-0000
@elaydis
Fix search result odering
6143d3a 
RISDEV-0000
@leonie-koch
Fix layout and search result display
b4d14c7 
RISDEV-3581
@leonie-koch
Change tree layout
a65f89f 
RISDEV-5344
@leonie-koch
Fix frontend test
f4c32b6 
RISDEV-0000
@elaydis
Fix behaviour with empty search inputs
db6980c 
RISDEV-5347
@elaydis
Tree styling
7760379 
RISDEV-5344
@elaydis
Styling
44381f1 
RISDEV-5344
@elaydis
Merge branch 'main' into field-of-law
ca071eb 
# Conflicts:
#	frontend/src/services/comboboxItemService.ts
@elaydis
More styling and skip tests
f32c302 
RISDEV-5344
@elaydis
Bring back dividers and make them blue
5d2e421 
RISDEV-5344
@elaydis
Field of law editable list styling
667bad3 
RISDEV-5414
elaydis added 18 commits November 20, 2024 12:43
@elaydis
Fix and add backend unit tests
6a7ab63 
RISDEV-3581
@elaydis
Add backend integration tests
3adc93a 
RISDEV-3581
@elaydis
Comment out tests
4a7c37f 
RISDEV-3581
@elaydis
Renaming to make it more clear when fol is added
28f7690 
RISDEV-3581
@elaydis
Remove log
760e0e1 
RISDEV-3581
@elaydis
Fix node highlighting
1e33132 
RISDEV-5342
@elaydis
Fix select from tree
cdc4c46 
RISDEV-3581
@elaydis
Display error label when searching without input
c040f66 
RISDEV-3581
@elaydis
Bring back field of law direct input
24a581e 
RISDEV-3581
@elaydis
Update backend tests field of law
8c0efe7 
RISDEV-3581
@elaydis
Show radio buttons when sachgebietslist is empty
40da214 
RISDEV-3581
@elaydis
Update text and frontend unit test
32dcfe0 
RISDEV-3581
@elaydis
Add reset search button
b6197fc 
RISDEV-5541
@elaydis
Refactor reset button and add test
2b52566 
RISDEV-5541
@elaydis
Merge branch 'main' into field-of-law
e74b833 
# Conflicts:
#	frontend/src/components/FieldOfLawMain.vue
@elaydis
Fix prettier
9091eb8 
RISDEV-0000
@elaydis
Fix layout from feedback
0bb4513 
RISDEV-3581
@elaydis
Fix radio button label size
420726e 
RISDEV-3581
@kiwikern kiwikern self-requested a review November 27, 2024 10:21
kiwikern
kiwikern approved these changes Nov 27, 2024
View reviewed changes
Copy link
Contributor

@kiwikern kiwikern left a comment

Choose a reason for hiding this comment

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

@elaydis @leonie-koch Wow, I am very impressed! This is such a huge improvement, using this component feels great. Great job! 👏 I left a lot of comments, feel free to resolve them without comment/explanation if not relevant. :-)

frontend/src/components/field-of-law/FieldsOfLaw.vue Outdated Show resolved Hide resolved
frontend/src/components/field-of-law/FieldsOfLaw.vue Outdated Show resolved Hide resolved
frontend/src/components/field-of-law/FieldOfLawSearchInput.vue Outdated Show resolved Hide resolved
frontend/src/components/field-of-law/FieldOfLawSearchInput.vue Outdated Show resolved Hide resolved
frontend/src/components/field-of-law/FieldOfLawSummary.vue Outdated Show resolved Hide resolved
frontend/test/components/periodicalEvaluation/handover/periodicalEditionHandover.spec.ts Outdated Show resolved Hide resolved
...end/src/main/java/de/bund/digitalservice/ris/caselaw/adapter/database/jpa/FieldOfLawDTO.java Outdated Show resolved Hide resolved
...e/bund/digitalservice/ris/caselaw/adapter/database/jpa/PostgresFieldOfLawRepositoryImpl.java
@@ -88,14 +88,14 @@ public List<FieldOfLaw> findBySearchTerms(String[] searchTerms) {
}

return listWithFirstSearchTerm.stream()
.filter(fieldOfLawDTO -> returnTrueIfInTextOrIdentifier(fieldOfLawDTO, searchTerms))
.filter(fieldOfLawDTO -> returnTrueIfInText(fieldOfLawDTO, searchTerms))
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Why do we query the database with only one search term instead of all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm not sure if I understand the question correctly 🤔 you mean write a query that directly queries with identifier and text combined in the database?

Copy link
Contributor

Choose a reason for hiding this comment

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

Above we query the database with only the first searchTerm:

repository.findAllByNotationAndIdentifierContainingIgnoreCaseOrTextContainingIgnoreCase(
            searchTerms[0])

Then we filter the results in memory.

An alternative would be to directly filter for multiple searchTerms in the database (the same we do for the first search term, but for each term and then connected via AND)

Copy link
Contributor

Choose a reason for hiding this comment

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

I just checkend and apparently the jpa method name magic does not support arrays, so you would need either native SQL or better use the Criteria API. -> The same as comment above, we can ignore this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

💡 thanks for checking this! criteria API sounds really promising

backend/src/main/java/de/bund/digitalservice/ris/caselaw/domain/FieldOfLawRepository.java
@@ -18,9 +18,16 @@ public interface FieldOfLawRepository {

List<FieldOfLaw> findBySearchTerms(String[] searchTerms);

List<FieldOfLaw> findByNormStr(String normStr);
List<FieldOfLaw> findByNorm(String normStr);
Copy link
Contributor

Choose a reason for hiding this comment

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

non-blocking idea: Instead of having specific methods for all, write one method with the criteriaAPI and dynamically construct the query with the search parameters that are present.

...nd/digitalservice/ris/caselaw/adapter/database/jpa/PostgresFieldOfLawRepositoryImplTest.java Outdated Show resolved Hide resolved
@elaydis
Copy link
Contributor Author

elaydis commented Nov 27, 2024

@kiwikern again thank you so much for taking the time and leaving all those helpful comments 🫶 I will merge now and look into improving the backend tomorrow so that users already get the new experience they were promised ☺️

@elaydis elaydis merged commit 0145335 into main Nov 27, 2024
26 checks passed
@elaydis elaydis deleted the field-of-law branch November 27, 2024 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kiwikern kiwikern kiwikern approved these changes

Assignees
No one assigned
Labels
dev-env Automatically spins up developer environment
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@elaydis @kiwikern @leonie-koch