Skip to content

Daterange#47

Closed
ipaksc wants to merge 5 commits intoServiceCanada:mainfrom
ipaksc:daterange
Closed

Daterange#47
ipaksc wants to merge 5 commits intoServiceCanada:mainfrom
ipaksc:daterange

Conversation

@ipaksc
Copy link
Contributor

@ipaksc ipaksc commented Jun 18, 2025

No description provided.

Copy link
Contributor

@GormFrank GormFrank left a comment

Choose a reason for hiding this comment

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

@ipaksc Thank you for opening a PR! I did a review and there are changes required.

Comment on lines +626 to +628
if ( urlParams.startdate && urlParams.enddate ) {
aqString += ' @date = "' + urlParams.startdate.replaceAll('-','/') + '..' + urlParams.enddate.replaceAll('-','/') + '"';
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@ipaksc Can you also include the cases where there would only be a startdate provided, and when there is only an enddate provided?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. i would
    replace
		if ( urlParams.startdate ) {
			aqString += ' @date = "' + urlParams.startdate.replaceAll('-','/') + '..' + currentDate() + '"';
		}

with

		if ( urlParams.startdate ) {
			aqString += ' @date >="' + urlParams.startdate.replaceAll('-','/')  + '"';
		}
  1. shouldn't the enddate be inclusuve
		if ( urlParams.enddate ) {
			aqString += ' @date  <=  "' + urlParams.enddate.replaceAll('-','/') + '"';
		}

src/connector.js Outdated
Comment on lines +633 to +643
if( declaredtype === "documents d'information" ){
declaredtype = "documents d&#39;information";
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@ipaksc What are you trying to accomplish exactly here? What's happening at the source that forces you to change the single quote to its HTML entity instead? Then, please evaluate if the code needs to be adapted, or if it should be the source instead.

If it's the source, then let's try to keep this as generic and flexible as possible, by determining if the source would ALWAYS have ' instead of '; and if that's the case then you should simply do a something like this instead of an IF condition:

Suggested change
if( declaredtype === "documents d'information" ){
declaredtype = "documents d&#39;information";
}
declaredtype = declaredtype.replace( /'/g, '&#39;' );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@GormFrank The request comes as "documents d'information" we have to match "documents d'information" in the index or the filter will not work.

image

@ipaksc
Copy link
Contributor Author

ipaksc commented Jul 25, 2025

PR #49 supersedes this one, so I am going to close this pull request.

@ipaksc ipaksc closed this Jul 25, 2025
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