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

Validate for Duplicate Event Entries within Multiple Offering Modal #1327

Open
wants to merge 20 commits into
base: development
Choose a base branch
from

Conversation

hoerstl
Copy link
Contributor

@hoerstl hoerstl commented Aug 28, 2024

Issue Description

There is no form validation preventing a user from creating multiple of the same event using the multiple offerings modal. Currently, when they try, the form submits and returns a warning that the "event already exists" because it is a duplicate event. This is not intended behavior and the user should be warned beforehand.

Fixes issue #1306

Changes

  • Added form validation which ensures that multiple offerings of events cannot be saved if their name, date, and start times are all the same (at least one must be different)
  • Highlights similar events in red if you try to save with duplicate events
  • Refactored previous form validation code for readability

Dev Note: After careful consideration, we determined that we should prevent a user from creating two offerings of an event if they had the same name, date, start time, even when they have different end times. We cannot think of a situation when two events wouldn't be the same even with different end times.

Testing

First, prepare your local development environment and run the application
Preparation:

  • source setup.sh
  • database/reset_database.sh test
  • flask run

Now, you should create an event under any program and click the slider to give the event multiple offerings.
image
In the modal, do your best to create duplicate events and save the event page. If you cannot create multiple offerings so that when you save the event, you get a warning message saying that the "event already exists", then our PR is working.
Try creating two events that have the same fields but differ once in each of the following areas per test:
All fields same except event name
All fields same except event date
All fields same except start time

e.x.
image

@hoerstl
Copy link
Contributor Author

hoerstl commented Sep 4, 2024

@hoerstl hoerstl marked this pull request as ready for review September 4, 2024 19:40
@bledsoef bledsoef self-requested a review September 6, 2024 17:40
Copy link
Contributor

@bledsoef bledsoef left a comment

Choose a reason for hiding this comment

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

Great work altogether! The only issue is an edge case where we get an error when trying to create an event in the multiple event offerings modal when one already exists in the database. It should probably prevent you from creating it in the first place. @stevensonmichel speak more with Lawrence about this he understands how it works.


event_dates = [event_data[0].startDate.strftime('%m/%d/%Y') for event_data in savedEventsList]
elif len(savedEvents) >= 1 and eventData.get('isMultipleOffering'):
event_dates = [event_data.startDate.strftime('%m/%d/%Y') for event_data in savedEvents]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this camel case instead of snake?


event_list = ', '.join(f"<a href=\"{url_for('admin.eventDisplay', eventId=event.id)}\">{event.name}</a>" for event in modifiedSavedEvents)
event_list = ', '.join(f"<a href=\"{url_for('admin.eventDisplay', eventId=event.id)}\">{event.name}</a>" for event in savedEvents)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this camel case instead of snake?

@@ -95,6 +96,52 @@ def deleteAllRecurringEvents(eventId):
eventId = allRecurringEvents[0].id
return deleteEventAndAllFollowing(eventId)

def attemptSaveMultipleOfferings(eventData, attachmentFiles = None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Excellent code

@@ -105,7 +152,7 @@ def attemptSaveEvent(eventData, attachmentFiles = None, renewedEvent = False):
If it is not valid it will return a validation error.

Returns:
Created events and an error message.
The saved event Created events and an error message if an error occurred.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Reword to "The saved event, created events, and an error message if an error occured."

}
}

if (isEmpty){
Copy link
Contributor

Choose a reason for hiding this comment

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

Some of the code in this conditional and following conditionals seem to be redundant. Can we potentially create a new function that has a parameter that can be passed in to ('.invalidFeedback').text() since the rest is identical.

Copy link

View Code Coverage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants