Skip to content

Conversation

@glenwperry
Copy link
Contributor

@glenwperry glenwperry commented Mar 10, 2025

  • Add functions for GetHistoricalDetours
  • Tweaks for SyncRouteEditor pattern requests
  • Mostly changes to get this project working with Chrome Headless instead of PhantomJS

@glenwperry glenwperry force-pushed the feature/EN-10178-show-historical-detours branch from 7b6b921 to 2d508eb Compare March 10, 2025 16:52
@glenwperry glenwperry marked this pull request as ready for review March 11, 2025 00:22
@glenwperry glenwperry requested review from mprzygoc and wisc-gmv March 11, 2025 00:33
.getPatternForSyncRouteEditor(1)
.then(getPatternResponse => {
getPatternResponse.should.be.an('object');
// todo: flesh this out after payload shape is confirmed
Copy link
Contributor

@wisc-gmv wisc-gmv Mar 11, 2025

Choose a reason for hiding this comment

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

This should be updated before pushed into the main branch.

Also would be nice if the route ID was defined as a variable for readability

const patternId = 1;
[...].getPatternForSyncRouteEditor(patternId);

beforeEach(() => fetchMock.catch(503));
afterEach(fetchMock.restore);

it('should get all historical detours without date parameters', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a page limit by default? Should it get all historical detours?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implementing paging is a pain so in this first iteration we are retrieving all historical detours by default OR using the "count" parameter for the first x detours by startDate desc. We will likely have to add paging this year.


if (params.length > 0) {
endpoint += `?${params.join('&')}`;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This works. 👍

Browsers also have wide support for UrlSearchParams that can build parameters, too.

const paramsObj = { foo: "bar", baz: "bar" };
const searchParams = new URLSearchParams(paramsObj);

console.log(searchParams.toString()); // "foo=bar&baz=bar"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah you're right this is a little old fashioned. Updating

@wisc-gmv
Copy link
Contributor

wisc-gmv commented Mar 11, 2025

@glenwperry
Looks good. Waiting for this one comment to be resolved and then I can approve. Nice job getting it to run with ChromeHeadless!

Copy link
Contributor

@wisc-gmv wisc-gmv left a comment

Choose a reason for hiding this comment

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

Looks good!

@glenwperry glenwperry merged commit 178ebf9 into master Mar 11, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants