Only allow saving itineraries with transit and no rentals #266

binh-dam-ibigroup · 2020-10-30T14:47:28Z

This PR shows the 'Save trip' button in the NarrativeItineraries if the selected itinerary has a transit leg and no rental legs, hides the button otherwise. The PR also removes the 'Save this option' trip button from LineItinerary.

…ransit.

evansiroky

See comment.

evansiroky · 2020-10-30T18:22:45Z

lib/util/itinerary.js

+// TODO: move to OTP-UI + add tests?
+/**
+ * @returns true if at least one of the legs of the specified itinerary is a transit leg.
+ */
+export function itineraryHasTransit (itinerary) {
+  if (itinerary && itinerary.legs) {
+    for (const leg of itinerary.legs) {
+      if (coreUtils.itinerary.isTransit(leg.mode)) {
+        return true
+      }
+    }
+  }
+
+  return false
+}
+
+// TODO: move to OTP-UI + add tests?
+/**
+ * @returns true if at least one of the legs of the specified itinerary is a rental leg
+ *   (e.g. CAR_RENT, BICYCLE_RENT, MICROMOBILITY_RENT).
+ */
+export function itineraryHasRental (itinerary) {
+  if (itinerary && itinerary.legs) {
+    for (const leg of itinerary.legs) {
+      if (leg.mode.indexOf('_RENT') > -1) {
+        return true
+      }
+    }
+  }
+
+  return false
+}
+
+/**
+ * Determines whether an itinerary can be monitored.
+ * @returns true if at least one of the legs of the specified itinerary is
+ *   a transit leg, and none of the legs is a rental leg (e.g. CAR_RENT, BICYCLE_RENT, etc.).
+ */
+export function itineraryCanBeMonitored (itinerary) {
+  return itineraryHasTransit(itinerary) && !itineraryHasRental(itinerary)
+}


These 3 functions could probably be combined into one so that a single loop occurs which checks each leg as needed. Furthermore, the coreUtils.itinerary.hasRental could be used. Also, this must check if a ridehail leg exists (coreUtils.itinerary.hasHail can be used). Also, I noticed that lib/util/state.js has a duplicate function of itineraryHasTransit. This redundancy should be avoided. Regarding testing, I think making tests for the itineraryCanBeMonitored would be good.

Yeah, I totally missed on the duplicate in state.js but that function is no longer used for the purpose of this PR. The changes are in df6bf8d, and the tests in aa19444.

landonreed · 2020-11-02T22:08:04Z

lib/components/narrative/narrative-itineraries.js

-              {/* FIXME: only save if meets requirements (e.g., is transit + non-realtime mode) */}
-              {persistence && persistence.enabled
+              {/* Only save if itinerary has transit and no rental modes that cannot be guaranteed. */}
+              {persistence && persistence.enabled && itineraryCanBeMonitored(itineraries[activeItinerary])


So, I think we should maybe take this opportunity to encapsulate this button into its own component. There are a few states that could be represented here, that we haven't quite addressed yet:

User is not logged in - in this instance, I think we should have the SaveTripButton render something like: "Please sign in to save trip"

Persistence disabled - just return null

itin cannot be monitored - we should probably disable the button and for now show a tooltip saying: "To save a trip it must contain a transit leg and no rental vehicles legs." Not sure if that's a great message long-term though...

I agree it is probably the simpler solution for now.

Agree

How about for the disabled button text: 🚫 Cannot save? And for the tooltip, taken from the backend: "Only trips that include transit and no rentals or ride hailing can be saved/monitored."

landonreed

Save trip button probably warrants a component at this stage. Also, what Evan said.

…ests.

evansiroky · 2020-11-04T21:40:15Z

__tests__/util/itinerary.js

+
+    testCases.forEach(({ expected, itinerary, title }) => {
+      it(title, () => {
+        expect(itineraryCanBeMonitored(itinerary))[expected ? 'toBeTruthy' : 'toBeFalsy']()


Since itineraryCanBeMonitored always returns exactly true or false, the readability of this assertion could be improved to be expect(...).toBe(expected).

Thanks, it was otherwise copy/paste from another test... Updated in. 4ccc522.

evansiroky · 2020-11-04T21:45:25Z

lib/components/narrative/save-trip-button.js

+const mapDispatchToProps = (dispatch, ownProps) => {}
+
+export default connect(mapStateToProps, mapDispatchToProps)(SaveTripButton)


mapDispatchToProps is not a required argument, so this can be replaced with connect(mapStateToProps)(SaveTripButton)

Updated in 4ccc522.

evansiroky

See 2 comments.

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

lib/components/narrative/save-trip-button.js

+  const { persistence } = state.otp.config
+  const itineraries = getActiveItineraries(state.otp)
+  return {
+    itinerary: activeSearch && itineraries && itineraries[activeSearch.activeItinerary],


Opportunity to use getActiveItinerary(state.otp) which handles all of this other getActiveSearch/getActiveItineraries logic.

Thanks. Updated in 80b8a91.

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

lib/components/narrative/line-itin/line-itinerary.js

-        {user &&
-          <span className='pull-right'><LinkButton to='/savetrip'>Save this option</LinkButton></span>
-        }
-


Actually, I know this goes against what I previously stated, but we should keep the new SaveTrip button component here <SaveTripButton />. This was decided in conversation with Ritesh earlier last week.

Reinstated in 80b8a91.

dec353a uses <SaveTripButton /> in LineItinerary.

landonreed

Looks good. Conditional approval on a few final items.

Address PR comments. Reinstate trip button in LineItinerary.

…SaveTripButton)

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

🎉 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 28, 2020 12:55

fix(NarrativeItineraries): Show Save trip only for itineraries with t…

0379130

…ransit.

fix(NarrativeItineraries): Save trip with transit and no rentals only.

5db7f2e

refactor(LineItinerary): Remove 'Save trip' for line itinerary.

aa74905

binh-dam-ibigroup requested review from evansiroky and landonreed October 30, 2020 14:47

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

binh-dam-ibigroup mentioned this pull request Oct 30, 2020

Only persist trip with transit and no-rental itinerary ibi-group/otp-middleware#91

Merged

3 tasks

evansiroky requested changes Oct 30, 2020

View reviewed changes

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

landonreed reviewed Nov 2, 2020

View reviewed changes

landonreed requested changes Nov 2, 2020

View reviewed changes

landonreed removed their assignment Nov 2, 2020

binh-dam-ibigroup added 5 commits November 2, 2020 17:20

refactor(util/itinerary): Refactor itineraryCanBeMonitored.

df6bf8d

test(util/itinerary): Add tests for itineraryCanBeMonitored.

aa19444

refactor(util/itinerary): Add missing rental checks + corresponding t…

936824e

…ests.

fix(SaveTripButton): Extract component, handle trip/login states.

b353dcf

refactor(SaveTripButton): Render disabled style.

944d078

binh-dam-ibigroup requested review from evansiroky and landonreed November 3, 2020 21:57

binh-dam-ibigroup assigned landonreed and evansiroky and unassigned binh-dam-ibigroup Nov 3, 2020

evansiroky reviewed Nov 4, 2020

View reviewed changes

evansiroky requested changes Nov 4, 2020

View reviewed changes

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

refactor: Address comments for test, redux connect.

4ccc522

binh-dam-ibigroup requested a review from evansiroky November 4, 2020 22:40

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

evansiroky approved these changes Nov 5, 2020

View reviewed changes

evansiroky removed their assignment Nov 5, 2020

landonreed reviewed Nov 10, 2020

View reviewed changes

landonreed approved these changes Nov 10, 2020

View reviewed changes

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

binh-dam-ibigroup added 4 commits November 10, 2020 17:48

refactor(SaveTripButton):

80b8a91

Address PR comments. Reinstate trip button in LineItinerary.

refactor(LineItinerary): Use SaveTripButton (add pull-right style to …

dec353a

…SaveTripButton)

refactor(itineraries): Remove unused user prop.

15f2534

Merge branch 'dev' into only-save-itins-with-transit

a35e942

binh-dam-ibigroup merged commit 761e817 into dev Nov 13, 2020

binh-dam-ibigroup deleted the only-save-itins-with-transit branch November 13, 2020 13:59

evansiroky mentioned this pull request Nov 20, 2020

Next release #277

Merged

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

		const mapDispatchToProps = (dispatch, ownProps) => {}

		export default connect(mapStateToProps, mapDispatchToProps)(SaveTripButton)

Only allow saving itineraries with transit and no rentals #266

Only allow saving itineraries with transit and no rentals #266

Uh oh!

Conversation

binh-dam-ibigroup commented Oct 30, 2020

Uh oh!

evansiroky left a comment

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!

evansiroky Nov 4, 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!

evansiroky left a comment

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

evansiroky Nov 4, 2020 •

edited

Loading