Check itinerary existence #242

binh-dam-ibigroup · 2020-10-01T23:39:42Z

This PR depends on #239 and requires changes from ~~ibi-group/otp-middleware#77~~ ibi-group/otp-middleware#90.

It causes the trip settings UI to

check for trip availability for each day of the week,
provide a visual indication for days a trip is not available,
set the flag to false for days on which a trip is not available, so those updates are persisted upon saving. (If a trip becomes unavailable on a day after the user saves it -- e.g. service is dropped on weekends on a route -- if the user makes a change, those days would no longer be monitored).

evansiroky · 2020-10-02T03:23:22Z

lib/components/user/trip-basics-pane.js

+  async componentDidMount () {
+    // Check itinerary existence for all days.
+    const { accessToken, config, setFieldValue, values } = this.props
+
+    const itineraryCheckResult = await checkItinerary(
+      config.persistence.otp_middleware,
+      accessToken,
+      values
+    )


Since this check occurs every time the component mounts, it might be possible that an itinerary becomes impossible for an existing itinerary that was possible before. Is there a way to have this run only when creating a monitored trip?

That was my initial thought and It is possible from the UI, but it will currently fail on the backend (the preUpdateHook method will trigger an error if the trip does not exist for one of the monitored days). Should the backend be changed to accommodate that?

We should probably think about this a little more in depth. At some point this UI should also show when an itinerary is no longer possible which means updating things in the backend and then communicating that somehow to the frontend.

I went ahead and remove the trip existence enforcement when saving existing trips in ibi-group/otp-middleware@78e31e7. With commit aeb519f, the UI will still perform the check and highlight the days an existing trip is not possible, however it will let the user save that trip. Trip existence continues to be enforced for new trips.

evansiroky

Until we figure out how to communicate that a Monitored Trip is no longer possible on a particular day of the week, we should have this itinerary existence check run only upon the creation of a new Monitored Trip.

binh-dam-ibigroup · 2020-10-16T21:12:28Z

Until we figure out how to communicate that a Monitored Trip is no longer possible on a particular day of the week, we should have this itinerary existence check run only upon the creation of a new Monitored Trip.

@evansiroky Did you mean also not enforcing that in the backend?

…is not available.

lib/components/user/trip-basics-pane.js

evansiroky

Seems good

lib/components/narrative/tabbed-itineraries.js

lib/util/middleware.js

… for itin checks.

landonreed · 2020-11-10T20:19:33Z

lib/actions/user.js

 const setLastPhoneSmsRequest = createAction('SET_LAST_PHONE_SMS_REQUEST')
 export const setPathBeforeSignIn = createAction('SET_PATH_BEFORE_SIGNIN')
+export const clearItineraryExistence = createAction('CLEAR_ITINERARY_AVAILABILITY')
+const setitineraryExistence = createAction('SET_ITINERARY_AVAILABILITY')


nitpick: I think the action string should match the action name i.e., replace AVAILABILITY with EXISTENCE.

Also setitineraryExistence has a typo (the first i should be capitalized).

Updated in 846892a.

landonreed · 2020-11-10T20:22:29Z

lib/actions/user.js

+export function checkItineraryExistence (trip) {
+  return async function (dispatch, getState) {
+    const { accessToken, apiBaseUrl, apiKey } = getMiddlewareVariables(getState())
+    const requestUrl = `${apiBaseUrl}${API_MONITORED_TRIP_PATH}${API_MONITORED_TRIP_CHECK_SUBPATH}`


I don't think API_MONITORED_TRIP_CHECK_SUBPATH needs to be in a constant if it's only used one time. Having constants far away from the place they're used can make things hard to read.

Sure. Updated in 846892a

landonreed · 2020-11-10T20:26:28Z

lib/components/user/trip-basics-pane.js

+const mapStateToProps = (state, ownProps) => {
+  const { accessToken, itineraryExistence } = state.user
+  return {
+    accessToken,


Is accessToken needed still?

Good find. Updated in 846892a.

landonreed · 2020-11-10T20:30:26Z

lib/components/user/trip-basics-pane.js

+ * @returns true if there is a trip matching for the specified availability/existence check.
+ */
+function itineraryExists (dayCheck) {
+  return dayCheck && dayCheck.valid


I'm not sure this method is needed any longer, is it? We should be guaranteed to have an entry for each day in the ItineraryExistence response, right?

No it is not. Updated in 846892a.

landonreed · 2020-11-10T20:33:49Z

lib/components/user/trip-basics-pane.js

+          <FormGroup validationState={monitoredDaysValidationState}>
+            <ControlLabel>What days do you take this trip?</ControlLabel>
+            <div>
+              {allDays.map(({ name, fullText, text }, index) => {


It seems a bit redundant to have allDays and ALL_DAYS, which effectively contain the same info.

Updated in 846892a.

landonreed

Just a few nitpicks. Otherwise, looks great!

github-actions · 2021-01-08T16:08:29Z

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

binh-dam-ibigroup added 3 commits October 1, 2020 16:09

refactor(middleware): Test a query.

2cc9825

feat(TripBasicsPane): Grey out days on which a trip is not available.

cbf3feb

fix(TripBasicsPane): Don't monitor days on which trips do not exist.

8aad883

binh-dam-ibigroup added the BLOCKED Blocked (waiting on another PR to be merged) label Oct 1, 2020

binh-dam-ibigroup requested review from evansiroky and landonreed October 1, 2020 23:40

binh-dam-ibigroup assigned landonreed and evansiroky Oct 1, 2020

evansiroky reviewed Oct 2, 2020

View reviewed changes

Base automatically changed from form-validation to dev October 9, 2020 17:47

evansiroky requested changes Oct 10, 2020

View reviewed changes

evansiroky assigned binh-dam-ibigroup and unassigned evansiroky Oct 10, 2020

binh-dam-ibigroup removed the BLOCKED Blocked (waiting on another PR to be merged) label Oct 26, 2020

binh-dam-ibigroup added 2 commits October 26, 2020 15:57

Merge branch 'dev' into check-itinerary-existence

68ffa44

refactor(TripBasicsPane): Let user select/save days an existing trip …

aeb519f

…is not available.

binh-dam-ibigroup requested a review from evansiroky October 26, 2020 20:56

binh-dam-ibigroup assigned evansiroky and unassigned binh-dam-ibigroup Oct 26, 2020

binh-dam-ibigroup added 2 commits October 26, 2020 17:15

refactor(TripBasicsPane): Tweak text.

b8faf63

refactor(TripBasicsPane): Tweak text.

cf3d214

evansiroky reviewed Oct 26, 2020

View reviewed changes

lib/components/user/trip-basics-pane.js Outdated Show resolved Hide resolved

evansiroky approved these changes Oct 26, 2020

View reviewed changes

evansiroky removed their assignment Oct 26, 2020

refactor(TripBasicsPane): Fix typo.

38b2249

landonreed reviewed Oct 28, 2020

View reviewed changes

lib/components/narrative/tabbed-itineraries.js Outdated Show resolved Hide resolved

landonreed reviewed Oct 28, 2020

View reviewed changes

lib/util/middleware.js Outdated Show resolved Hide resolved

landonreed reviewed Oct 28, 2020

View reviewed changes

lib/util/middleware.js Outdated Show resolved Hide resolved

refactor(TripBasicsPane): Adjust to new backend return data structure…

b0831d1

… for itin checks.

Merge branch 'dev' into check-itinerary-existence

1cbc87a

binh-dam-ibigroup assigned binh-dam-ibigroup and unassigned landonreed Oct 29, 2020

binh-dam-ibigroup added 4 commits October 29, 2020 21:21

refactor(actions/user): Move itinerary check to actions/reducer.

ea97ce6

fix(NarrativeItineraries): Adjust Proptypes.

214e266

refactor: Adjust to updated itinerary check backend.

efa6a65

refactor(actions/user): Twaek comment/error msg for itinerary check.

e7cb719

binh-dam-ibigroup requested a review from landonreed November 4, 2020 19:36

binh-dam-ibigroup assigned landonreed and unassigned binh-dam-ibigroup Nov 4, 2020

landonreed reviewed Nov 10, 2020

View reviewed changes

landonreed requested changes Nov 10, 2020

View reviewed changes

landonreed assigned binh-dam-ibigroup and unassigned landonreed Nov 10, 2020

refactor(TripBasicsPane,actions/user): Address PR comments.

846892a

binh-dam-ibigroup requested a review from landonreed November 10, 2020 22:36

binh-dam-ibigroup assigned landonreed and unassigned binh-dam-ibigroup Nov 10, 2020

landonreed approved these changes Nov 10, 2020

View reviewed changes

landonreed merged commit 96d89d1 into dev Nov 10, 2020

landonreed deleted the check-itinerary-existence branch November 10, 2020 23:03

evansiroky mentioned this pull request Nov 20, 2020

Next release #277

Merged

github-actions bot added the released label Jan 8, 2021

Check itinerary existence #242

Check itinerary existence #242

Uh oh!

Conversation

binh-dam-ibigroup commented Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

binh-dam-ibigroup Oct 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evansiroky left a comment

Choose a reason for hiding this comment

Uh oh!

binh-dam-ibigroup commented Oct 16, 2020

Uh oh!

Uh oh!

evansiroky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

landonreed Nov 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

landonreed left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

binh-dam-ibigroup commented Oct 1, 2020 •

edited

Loading

binh-dam-ibigroup Oct 26, 2020 •

edited

Loading

landonreed Nov 10, 2020 •

edited

Loading