Skip to content

Feature/EN-10178-show-historical-detours #134

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

Merged
merged 25 commits into from
Mar 11, 2025

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 wisc-gmv and mprzygoc 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.

2 participants