USGS-WiM / whispers

Wildlife Health Information Sharing Partnership event reporting system(WHISPers) v2
Other
2 stars 1 forks source link

org/species/location required fields not enforced on event details page #1238

Closed JChipault closed 1 year ago

JChipault commented 3 years ago

Describe the bug On create event, you have to have an event organization, a location, and at least one species. This is good, because these field are required. However, on the event details page, you can delete all the event orgs, you can delete all the species, and you can even delete all the locations and still mark an event complete.

To Reproduce Steps to reproduce the behavior:

  1. Go to the event details page of an incomplete event on the test site that has an org and a species (and thus a location as well)
  2. Delete all event orgs
  3. Delete all species
  4. Switch event to complete.
  5. Note that there's no error message about missing an event org or species
  6. Switch event back to incomplete
  7. Delete location
  8. Switch event to complete
  9. Note that there's not error message about missing a location

Expected behavior If you try to delete the last remaining org, species, or location on the event details page, you get a message that tells you that you have to add a new org/spec/loc before you can delete this only remaining one. Maybe after someone clicks "Yes, Delete Event Organization" in the "Delete Event Organization Confirm" popup for the last remaining org, then red text tells them something like "An event must have at least one event organization. Please enter another event organization before deleting."

Same goes for deleting the last species or the last location.

This would be similar to the red text message users get when they try to delete the only number associated with a species at a location (see below for what I get when I deleted "1" in the "known dead" field on the event details page):

image

JChipault commented 3 years ago

To be clear, we'd prefer that a user is told in real-time that the field is required instead of waiting to run those business rules to check for an org/species/location after an event is marked "complete". Lots of events linger in the "incomplete" stage and we'd want those events to have this required information at all times. I just put the toggle to "complete" in the steps to reproduce to show that these business rules weren't even being run at event completion.

JChipault commented 3 years ago

Alternatively, perhaps the "Yes, Delete Event Organization" in the "Delete Event Organization Confirm" popup for the last remaining org is greyed out (i.e., button not active) and there's red text telling the user something like "An event must have at least one event organization. Please enter another event organization before deleting."

BlakeDraper commented 3 years ago

This is a very reasonable thing to request - it is the business rules after all. But it will be a bit of a lift that probably requires deferral until next FY.

A little background on addressing this issue: The event submission form is easier to analyze across the entirety of the data entered because it is all one form. I have written functions that validate individual values, as well as certain choices as they relate to others. The result of that is those red text warnings and the form remaining invalid/not submit-able until the business rule violations are addressed.

The event details page on the other hand, is not one form. It is a bunch of event data taken from the database, organized and displayed. By this point the data is more database-structured in the sense that each of those items (event, event location, event location species, species diagnosis) are all distinct (but related) objects in the database. In turn, the various ways to edit/update those items on the event details page operate somewhat independently. I offer that as mere explanation, not as an excuse to not meet the request.

So, in order to have the pan-event 'awareness' like the event submission form has, we'll need to sort of re-compose all that data into a reference object that can then be validated against. I'm not sure how long that will take, but it definitely will be at least a bit more than the ~4 hours I have remaining in WHISPers hours this FY.

The end of the FY always gets a little fuzzy in September - so I may be able to steal some time back to work on this, but just FYI it may need to wait until a renewed set of hours arrives in October.

JChipault commented 3 years ago

Thanks for this explanation, Blake. I connected with Neil and we don't think it's worth 4+ hrs currently, especially since it's possible that the event details page will get a bit of an overhaul during the next round of development.

Neil had a couple of thoughts... 1) Could Aaron put it at the web services layer? Neil thinks it's less involved at the web services layer, but the user is only going to get a toast pop-up. 2) There are 3 checks here; could we split them up and just do event org for now? Do you have a guess at how much time that would take? Of these 3, missing event org appears to be the most commonly violated in the system.

BlakeDraper commented 3 years ago
  1. Theoretically it is less involved at that level because there isn't all the user prompts and warnings to deal with, but I think the same challenge exists of needing to assemble the full event data to reference for validation (tho I will let @aaronstephenson weigh in). That of course will be a bad UX with the user not knowing something is wrong until the end when they submit.

  2. Checking for just event org is pretty easy. Can do that with just an hour or two. I will create an issue for that.

JChipault commented 3 years ago
  1. Don't worry much about this.
  2. Awesome! This will help a lot
aaronstephenson commented 3 years ago

Just seeing this now (returning from annual leave).

Re: #1, yes it would actually require a fair bit of work to change the web services, for the reasons Blake enumerated. The "child" objects don't have the same validations as the parent event, because they're being updated/deleted in isolation (not as a giant package of Event data + child object data). We can certainly change that behavior (and probably should, eventually) by looking up the parent and sibling objects and running the business rule validations against all the "recombined" related data.

JChipault commented 3 years ago

ok, let's hold on 1 then. Thanks!

BlakeDraper commented 2 years ago

@JChipault I added this check which prevents the deletion of the last species from an event location by disabling the delete button and informing the user why they cannot delete it. Let me know what different text you would like there, if any.

image

JChipault commented 2 years ago

Would that "!" message show any time someone looks at a location that only has one species? Or does the "!" message only show up if someone tries to click "delete species" when there is only one species?

BlakeDraper commented 2 years ago

@JChipault As written it would show when they expand the location species record to look at it, so yes it is passive or 'just there', but also not visible without a click first. Do you want to lose it? Or just lose the exclamation icon?

JChipault commented 2 years ago

Is there a way to have that message show only if they try to click delete? Could we leave delete looking active when there's only one species, but then if they click delete then show this "!" and don't actually delete? Most of our locations only have one species so, if passive, this red text is likely going to feel like a warning on most viewings of the data, even though nothing is wrong with only have one species.

JChipault commented 2 years ago

I'm open to other workflows, but would prefer the "!" message (in it's entirety, not just talking about the exclamation point) only shows when a user tries to delete that last species, and does not show when they are just viewing a location that has one species in it (which is most locations)

BlakeDraper commented 2 years ago

@JChipault Is it ok to just have "Delete" disabled when only one species? Or do you want the explanation text too? A disabled button should do nothing. Instead of disabling the button we could also do a pop-up with that text.

JChipault commented 2 years ago

For consistency in user experience, maybe we just follow what was done for event organization - the delete button is active and the user gets a pop-up with text.

And our text here could follow the text for event org: At least one species is required for all locations. You are attempting to delete the only associated species. Please add an additional species before deleting [fill in the blank species].

image

JChipault commented 2 years ago

Deletion of all event orgs and all species at a location is being prevented nicely now on the test site. We've decided to wait on preventing deletion of all locations.

BlakeDraper commented 1 year ago

Published in v2.17.0