Skip to content

Scc 5348, 5353, 5354#731

Open
danamansana wants to merge 10 commits intomainfrom
scc-5348
Open

Scc 5348, 5353, 5354#731
danamansana wants to merge 10 commits intomainfrom
scc-5348

Conversation

@danamansana
Copy link
Copy Markdown
Contributor

  • update ES fields for series and genre to match upcoming work in advanced search
  • update date fields:
    • > should be greater than the entire date, i.e. gte the following date
    • <= should be lte the entire date, i.e. lt the following date
    • within is the same logic as <=
    • encloses was just a mistake, it is updated to look for bibs with a range of dates including the given date
  • I've added an experimental wildcard search
  • New error class: InvalidQuerySyntaxError which triggers a 422 response in case of incorrect syntax
  • Fix the tests

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit. These aren't really fixtures, are they?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm perhaps I am using that term incorrectly. What would you call them?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To my mind, fixtures are the static data you initialize the system with to ensure your input has a consistent output. They are the data you use to stabalize the parts of the system you're not testing so that you can focus on the part you are testing by making assertions about specific inputs having specific outputs. Seems like you just want to maintain your set of expectations in a different file from the tests, but that doesn't make them fixtures. They're the interesting parts, actually. Maybe just "cql generated es queries"?

]
}
}
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • These are the original marc subfield tags? Why restrict to these? Is it not possible to do a encloses match on an e tagged date - or any other non-null date for that matter?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Per Sean encloses should only match bibs with date range types

'parallelSeriesAddedEntry.folded'
],
exact_fields: [
'series',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

these should be updated to *.keywordLowercasedStripped once the new es index is popuated. i can add it to my ticket to update discovery API

.filter(fieldMatcher(queryTerm))
.map(field => field.value)

if (matchingValues.length === 0) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

in what case would these values have length zero?

const match = date.match(dateReg)
let rangeEnd
function nextDate (dateString) {
const dateRegex = /^(\d{4})(?:[-/](\d{2}))?(?:[-/](\d{2}))?$/
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

seems like this method is responsible for validating dates. is there a specific case here that you're trying to account for? maybe you can pass the string into new Date() without regex crunching?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this is the about all of the ES date range shenaningans, but i think some comments would help to describe the transformations you're making

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.

3 participants