Skip to content
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

Propagate delete companion #278

Merged
merged 5 commits into from
Dec 19, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 26 additions & 7 deletions src/main/java/org/opentripplanner/middleware/models/OtpUser.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import com.fasterxml.jackson.annotation.JsonGetter;
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonSetter;
import com.mongodb.client.model.Filters;
import org.opentripplanner.middleware.auth.Auth0Users;
import org.opentripplanner.middleware.auth.RequestingUser;
import org.opentripplanner.middleware.persistence.Persistence;
Expand All @@ -21,7 +22,11 @@
import java.util.stream.Collectors;
import java.util.stream.Stream;

import static org.opentripplanner.middleware.tripmonitor.TrustedCompanion.invalidateRelatedUsers;
import static org.opentripplanner.middleware.tripmonitor.TrustedCompanion.removeCompanion;
import static org.opentripplanner.middleware.tripmonitor.TrustedCompanion.removeDependent;
import static org.opentripplanner.middleware.tripmonitor.TrustedCompanion.removeObserver;
import static org.opentripplanner.middleware.tripmonitor.TrustedCompanion.removePrimaryTraveler;

/**
* This represents a user of an OpenTripPlanner instance (typically of the standard OTP UI/otp-react-redux).
Expand Down Expand Up @@ -143,20 +148,34 @@ public boolean delete(boolean deleteAuth0User) {
// If a related user, invalidate relationship with all dependents.
for (String userId : dependents) {
OtpUser dependent = Persistence.otpUsers.getById(userId);
if (dependent != null) {
for (RelatedUser relatedUser : dependent.relatedUsers) {
if (relatedUser.email.equals(this.email)) {
relatedUser.status = RelatedUser.RelatedUserStatus.INVALID;
}
}
if (dependent != null && invalidateRelatedUsers(email, dependent.relatedUsers)) {
Persistence.otpUsers.replace(dependent.id, dependent);
}
}

// If a dependent, remove relationship with all related users.
// If a dependent user, remove relationship with all related users.
for (RelatedUser relatedUser : relatedUsers) {
removeDependent(this, relatedUser);
}

// If a dependent user, remove self as the primary traveler in trips created by other users.
// TODO: Should we alert the user who created the trip of the deletion?
Persistence.monitoredTrips
Copy link
Contributor

Choose a reason for hiding this comment

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

There is probably a case here for just deleting the trip if the primary traveler is no longer travelling. Perhaps that would start to complicate things though?!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good observation. Could preventing tracking if a companion is set but no primary traveler is defined be a better way to handle that?

Choose a reason for hiding this comment

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

I think deleting the trip makes sense if there is no primary traveler.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Created issue #282 to track that.

.getFiltered(Filters.eq("primary.userId", id))
.forEach(trip -> removePrimaryTraveler(this, trip));

// If a companion user, invalidate relationship in trips where they are companions and observers.
// TODO: Should we alert the user who created the trip of the deletion?
Persistence.monitoredTrips
.getFiltered(Filters.eq("companion.email", email))
.forEach(trip -> removeCompanion(this, trip));

// If a companion user, invalidate relationship in trips where they are companions and observers.

Choose a reason for hiding this comment

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

nit. this comment is a duplicate of the comment above.

// TODO: Should we alert the user who created the trip of the deletion?

Choose a reason for hiding this comment

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

I don't think it would hurt to notify the creator of the trip.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Created issue #283 to track that. The dilemma is to choose between #282 and #283.

Persistence.monitoredTrips
.getFiltered(Filters.eq("observers.email", email))
.forEach(trip -> removeObserver(this, trip));

return Persistence.otpUsers.removeById(this.id);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import org.opentripplanner.middleware.auth.Auth0Connection;
import org.opentripplanner.middleware.i18n.Message;
import org.opentripplanner.middleware.models.MobilityProfileLite;
import org.opentripplanner.middleware.models.MonitoredTrip;
import org.opentripplanner.middleware.models.OtpUser;
import org.opentripplanner.middleware.models.RelatedUser;
import org.opentripplanner.middleware.persistence.Persistence;
Expand All @@ -21,6 +22,7 @@
import java.net.URLEncoder;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
Expand Down Expand Up @@ -248,6 +250,62 @@ public static void removeDependent(OtpUser dependent, RelatedUser relatedUser) {
}
}

/**
* Remove the specified user as the primary traveler from the specified trip.
*/
public static void removePrimaryTraveler(OtpUser otpUser, MonitoredTrip trip) {
if (trip.primary != null && otpUser.id.equals(trip.primary.userId)) {
trip.primary = null;
Persistence.monitoredTrips.replace(trip.id, trip);
}
}

/**
* Remove the specified user as companion from the specified trip.
*/
public static void removeCompanion(OtpUser otpUser, MonitoredTrip trip) {
if (invalidateRelatedUser(otpUser.email, trip.companion)) {
Persistence.monitoredTrips.replace(trip.id, trip);
}
}

/**
* Remove the specified user as observer from the specified trip.
*/
public static void removeObserver(OtpUser otpUser, MonitoredTrip trip) {
if (invalidateRelatedUsers(otpUser.email, trip.observers)) {
Persistence.monitoredTrips.replace(trip.id, trip);
}
}

/**
* Helper to invalidate a RelatedUser (companion) relationship.
* @return true if the status changed to invalid, false otherwise.
*/
public static boolean invalidateRelatedUser(String email, RelatedUser relatedUser) {
if (
relatedUser != null &&
email.equals(relatedUser.email) &&
relatedUser.status == RelatedUser.RelatedUserStatus.CONFIRMED
) {
relatedUser.status = RelatedUser.RelatedUserStatus.INVALID;
return true;
}
return false;
}

/**
* Remove the specified user as observer from the specified trip.
* @return true if one or more related users gor invalidated, false otherwise.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit. are not gor.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! 528d8bb

*/
public static boolean invalidateRelatedUsers(String email, Collection<RelatedUser> relatedUsers) {
boolean hasChanged = false;
for (RelatedUser relatedUser : relatedUsers) {
hasChanged |= invalidateRelatedUser(email, relatedUser);
}
return hasChanged;
}

/**
* Retrieve the mobility profile for a dependent providing the requesting user is a trusted companion.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import org.junit.jupiter.params.provider.MethodSource;
import org.opentripplanner.middleware.models.MobilityProfile;
import org.opentripplanner.middleware.models.MobilityProfileLite;
import org.opentripplanner.middleware.models.MonitoredTrip;
import org.opentripplanner.middleware.models.OtpUser;
import org.opentripplanner.middleware.models.RelatedUser;
import org.opentripplanner.middleware.persistence.Persistence;
Expand All @@ -31,6 +32,8 @@

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assumptions.assumeTrue;
import static org.opentripplanner.middleware.auth.Auth0Connection.restoreDefaultAuthDisabled;
Expand Down Expand Up @@ -241,9 +244,47 @@ void canRemoveRelatedUserOnDelete() {
nickname
));
Persistence.otpUsers.replace(dependentUserThree.id, dependentUserThree);

// Create a monitored trip with dependentUserThree as primary traveler, relatedUserThree as companion.
MonitoredTrip tripWithPrimaryTraveler = new MonitoredTrip();
tripWithPrimaryTraveler.id = UUID.randomUUID().toString();
tripWithPrimaryTraveler.userId = relatedUserThree.id;
tripWithPrimaryTraveler.primary = new MobilityProfileLite(dependentUserThree);
tripWithPrimaryTraveler.companion = new RelatedUser(
relatedUserThree.email,
RelatedUser.RelatedUserStatus.CONFIRMED,
"nickname"
);
Persistence.monitoredTrips.create(tripWithPrimaryTraveler);

// Create a monitored trip with dependentUserThree as companion and observer to relatedUserThree.
// (In practice, a user cannot be both a companion and observer.)
MonitoredTrip tripWithCompanionAndObserver = new MonitoredTrip();
tripWithCompanionAndObserver.id = UUID.randomUUID().toString();
tripWithCompanionAndObserver.userId = relatedUserThree.id;
tripWithCompanionAndObserver.companion = new RelatedUser(
dependentUserThree.email,
RelatedUser.RelatedUserStatus.CONFIRMED,
"nickname"
);
tripWithCompanionAndObserver.observers.add(tripWithCompanionAndObserver.companion);
Persistence.monitoredTrips.create(tripWithCompanionAndObserver);

dependentUserThree.delete(false);
relatedUserThree = Persistence.otpUsers.getById(relatedUserThree.id);
assertFalse(relatedUserThree.dependents.contains(dependentUserThree.id));

// If a dependent user deletes their profile, delete them from any trip where they are a dependent.
MonitoredTrip updatedTripWithPrimaryTraveler = Persistence.monitoredTrips.getById(tripWithPrimaryTraveler.id);
assertNull(updatedTripWithPrimaryTraveler.primary);

// If a companion on a trip deletes their profile, invalidate them from any trip where they are a companion
// or an observer.
MonitoredTrip updatedTripWithCompanionAndObserver = Persistence.monitoredTrips.getById(tripWithCompanionAndObserver.id);
assertNotNull(updatedTripWithCompanionAndObserver.companion);
assertEquals(RelatedUser.RelatedUserStatus.INVALID, updatedTripWithCompanionAndObserver.companion.status);
assertFalse(updatedTripWithCompanionAndObserver.observers.isEmpty());
assertEquals(RelatedUser.RelatedUserStatus.INVALID, updatedTripWithCompanionAndObserver.observers.get(0).status);
}

/**
Expand Down
Loading