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

Infoj Order Correction #1508

Merged
merged 3 commits into from
Sep 27, 2024
Merged

Conversation

RobAndrewHurst
Copy link
Contributor

@RobAndrewHurst RobAndrewHurst commented Sep 27, 2024

Description

The lookup for the infoj_order checks on key, then field, and finally query. This fails when a key exists but doesn't match.

We have created a set of the different fields that will then be compared with a has check on the entry.

GitHub Issue

Type of Change

Please delete options that are not relevant, and select all options that apply.

  • ✅ Bug fix (non-breaking change which fixes an issue)
  • ✅ Testing

How have you tested this?

Run the local tests.

Testing Checklist

  • ✅ Existing Tests still pass
  • ✅ New Tests Added
  • ✅ Ran locally on my machine

@RobAndrewHurst RobAndrewHurst added Bug A genuine bug. There must be some form of error exception to work with. v4.x.x labels Sep 27, 2024
@RobAndrewHurst RobAndrewHurst self-assigned this Sep 27, 2024
@RobAndrewHurst RobAndrewHurst changed the title Infoj Order and test Infoj Order Correction Sep 27, 2024
@dbauszus-glx dbauszus-glx linked an issue Sep 27, 2024 that may be closed by this pull request
Copy link

sonarcloud bot commented Sep 27, 2024

Copy link
Member

@dbauszus-glx dbauszus-glx left a comment

Choose a reason for hiding this comment

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

Looking good.

I moved the map method into documented function to make this eassier readable.

image

I updated the test to be in order of infoj > order > execution > results > expected > test for readability especially when setting breakpoints.

image

Note that you can create an array from an elements list allowing to use map functions instead of pushing into results in a for loop.

      const results = Array.from(infoj.children)
        .map(el => el.firstChild.innerText.trim())

Copy link
Contributor

@AlexanderGeere AlexanderGeere left a comment

Choose a reason for hiding this comment

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

lgtm

@RobAndrewHurst
Copy link
Contributor Author

Looking good.

I moved the map method into documented function to make this eassier readable.

image

I updated the test to be in order of infoj > order > execution > results > expected > test for readability especially when setting breakpoints.

image

Note that you can create an array from an elements list allowing to use map functions instead of pushing into results in a for loop.

      const results = Array.from(infoj.children)
        .map(el => el.firstChild.innerText.trim())

Nice one!

@RobAndrewHurst RobAndrewHurst merged commit ae3632b into GEOLYTIX:main Sep 27, 2024
5 checks passed
@RobAndrewHurst RobAndrewHurst deleted the infoj_order branch September 27, 2024 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A genuine bug. There must be some form of error exception to work with. v4.x.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

infoj_order lookup
3 participants