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

Bug: Drupal: Events can be saved with Location type: VA facility, and empty Facility Location #17426

Open
2 tasks
Tracked by #17622
jilladams opened this issue Mar 5, 2024 · 10 comments
Open
2 tasks
Tracked by #17622
Assignees
Labels
Defect Something isn't working (issue type) Drupal engineering CMS team practice area Events product maintained by Public Websites team Public Websites Scrum team in the Sitewide crew

Comments

@jilladams
Copy link
Contributor

jilladams commented Mar 5, 2024

Description

#17424 found 2 Events that were published with the following location info:

  • Location type: At a VA facility
  • Facility location: Empty
    Screenshot 2024-03-05 at 3 41 32 PM

That shouldn't be allowed in the content model. VA Facility should require a Facility Location.

FE consequences

Not proven yet, but it appears that the missing data broke FE templates, potentially, and led to Ui you see in #17424 .

Acceptance Criteria

On an Event node:

  • Given Location type is set to At a VA facility, then Facility Location should be required
  • Given Location type is set to At a non-VA location, then Street address, City, and State should be required
@jilladams jilladams added Needs refining Issue status Drupal engineering CMS team practice area Public Websites Scrum team in the Sitewide crew Events product maintained by Public Websites team labels Mar 5, 2024
@FranECross FranECross removed the Needs refining Issue status label Mar 7, 2024
@jilladams jilladams changed the title Drupal: Events can be saved with Location type: VA facility, and empty Facility Location Bug: Drupal: Events can be saved with Location type: VA facility, and empty Facility Location Mar 18, 2024
@jilladams jilladams added the Defect Something isn't working (issue type) label Mar 18, 2024
@dsasser
Copy link
Contributor

dsasser commented Apr 16, 2024

PR is up: #17859

@dsasser
Copy link
Contributor

dsasser commented Apr 16, 2024

Status update 4/16/24

This work is in PR. I got a review by Christian who noted a very odd browser behavior which is allowing submitting despite having empty, required fields when saving an Event. The behavior which prevents the form from submitting empty, required fields, is native to the browser itself, so this may be challenging to troubleshoot, not to mention unexpected.

@dsasser
Copy link
Contributor

dsasser commented Apr 17, 2024

The bug mentioned here is pre-existing for Events, and possibly other forms in the CMS. Initial findings indicate it is a problem with a contributed module: Clientside Validation.

This may be an issue for the Platform CMS team, since that module isn't actively being used by the Event product. However, the opposite case could be made in that the Event product is the only product (known to be) experiencing the issue, and so the resolution should fall to us. @FranECross @jilladams any thoughts on that?

@FranECross
Copy link

@dsasser @jilladams I'm leaning toward asking the CMS team to handle it... reasoning is because we aren't actually using it for Events, I think it should fall on them to test to see if other products are actually (silently) having this issue, and then fix it for all product. But... I know they're a new team and don't know what their backlog is. Perhaps Jill can rock/paper/scissors this, or provide a different viewpoint? Thanks!

@jilladams
Copy link
Contributor Author

That setting is enabled in Prod: https://prod.cms.va.gov/admin/config/user-interface/clientside-validation

I understand from the PR notes:

  • With clientside validation ON: we experience the bug ( true in prod)
  • With clientside validation OFF: the browser doesn't intervene, and Drupal validation works as expected ?

So: yep, I agree: this is a CMS-wide config that is affecting Events form and probably others, and we should at least talk to them about it. (It may be that we are asked to handle fixing or figuring out next steps, if we have capacity and the negative impact on Events is worrisome for us.) I can spin up a thread.

@jilladams
Copy link
Contributor Author

https://dsva.slack.com/archives/CT4GZBM8F/p1713396251131369

@dsasser
Copy link
Contributor

dsasser commented Apr 25, 2024

Following up on the Clientside Validation problem:

It was decided to let this PR through before fixing the Clientside Validation problem, since the changes here are benign for existing Events, but fixes the issue for new Events.

@jilladams
Copy link
Contributor Author

@dsasser and it's true I think that our fix won't totally fix everything / this problem could recur until the Clientside validation fix is handled?

@dsasser
Copy link
Contributor

dsasser commented Apr 26, 2024

@dsasser and it's true I think that our fix won't totally fix everything / this problem could recur until the Clientside validation fix is handled?

That is correct. Once the Clientside Validation problem is resolved the solution we merge as part of this PR will then begin to function correctly when editing existing Events.

@jilladams
Copy link
Contributor Author

Noting: this work is done for all intents and purposes, but am keeping the ticket in Complete Pending Integration, to reflect that we will not be able to verify the final behavior until the blocking CMS ticket is complete (#17896). @FranECross @dsasser if you'd rather keep track of that some other way, I'm open to whatever, just let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Defect Something isn't working (issue type) Drupal engineering CMS team practice area Events product maintained by Public Websites team Public Websites Scrum team in the Sitewide crew
Projects
None yet
Development

No branches or pull requests

3 participants