-
Notifications
You must be signed in to change notification settings - Fork 70
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
Drupal: back end changes and associated tests for Address module upgrade to version 2 #17191
Comments
@xiongjaneg after chatting with Dave C I added the following AC to the description above (in efforts to move toward a consistent and repeatable process for teams working in CMS)
@EWashb for your awareness |
@FranECross for your awareness |
@xiongjaneg I created a testing ticket for the PW team, and also added your epic to it for visibility. Thanks! cc @dsasser @jilladams |
All three address lines should be enabled. |
@xiongjaneg For our sprint planning purposes, when do you estimate this will be ready for the PW team to test? Thanks in advance! cc @jilladams @dsasser |
@FranECross We'll get it into Sprint 105 so sometime that sprint, PW will get tagged on the PR to review in Tugboat. How does that sound? |
@xiongjaneg Sounds perfect! Thanks so much for the quick response. |
@laflannery @thejordanwood I'm interested in your feelings about the addition of this line in the address. The upgraded module has two additional lines, compared to one additional in the current version. This makes the UI feel a little bare without any label, but I'd like to know what you think. |
@omahane I'm not a huge fan. They do have sr-only labels but I do think some direction here would really help. Actually showing the labels I'm not sure how much help that would provide, because the labels are just "Street address line 2" and "Street address line 3". My preference would probably be to provide help text to let editors know what each field would be for. Is that an option? |
We can definitely look at that as a part of this ticket. |
Do you know where we could add the help text? would it be general text right under "Street address" or is adding individual text for each field a possibility? I'm saying saying I necessarily want to do that I'm jut exploring my options |
To be able to add help text via the UI (config), it looks like it only allows us to add it to the entire Address widget. So I think in the example above it would show the help text just above the 'Country' drop-down. However, we should be able to add help text to any field with code via form alters. I think the drawback to this is that the text is then hard-coded (potentially in multiple spots in the code-base) and a bit more difficult to find and change if needed. |
I talked with Jordan - Is it possible to just not display the 3rd address line? We don't see a reason why editors would need this and the more flexibility we give editors, the more they are going to ad extra and unnecessary information. There is also a chance they may think it's required, given the way that this is set up. If that's possible, that seems like the best option. Then we think the CMS team should handle a more overall, better experience for help text/labelling/etc. since they are technically owners of the module |
It is possible to hide the 3rd line, and this would actually simplify a lot of the work, since it's basically just making it work the same way it already was. There are instances where the data from the API has 3 lines of data and we have been concatenating that into 1 or 2 lines because there was no option of a 3rd line before. I don't think I see any issues with continuing that but would like to talk about it with everyone during a 16th minute if possible. |
@Becapa Not sure we'll have the people we want at 16th minute to discuss. I'll tag @davidmpickett to add to UX sync. |
Sample data from the Facilities API137 of 306 facilities with an address_3 entry also have address_2: null The others should give you an idea of how the address_3 field is being used upstream.
|
It was agreed upon in UX sync to hide the 3rd line in all places for now. This will essentially keep everything looking and working the same way it was before upgrading the module to version 2.0. This will also allow teams to take a look at their individual content types/fields and update them on a case-by-case basis. Because of this, I don't think the additional Front End ticket is needed any longer and I will update this ticket's description to reflect these changes. |
Merged to integration branch |
Here is the PR for the merge of the integration branch into main: #17393 |
@FranECross @gracekretschmer-metrostar Which engineers on your teams, would you like to review this ticket? The major change is to add an address line 3. Our recommendation and plan is to hide line 3 from Drupal editors to keep their experience the same as now. This could be changed later with further exploration of use cases. The ticket notes some use cases we're aware of for line 3. |
Closing the loop.... Daniel S on the PW team and Edmund on the CMS team reviewed > Slack thread 🎉 |
Description
This is part 2 of the Address module upgrade to version 2
In part 1 #14083, the work was done to review the release notes and upgrade the module in an integration branch while determining exactly what additional changes need to happen (i.e. the facility migrations, graphql queries, front end templates, and any additional changes found after performing the upgrade).
This ticket is for the back end changes along with any associated tests that should be added to the integration branch (separate ticket that is populated with the findings of this ticket).
This precedes part 3, the front end changes along with any associated tests should be added to the integration branch (another separate ticket that is populated with the findings of this ticket).Additional information
The Address module is inline to release a 2.0 version of the module.
Possible issues (just for advanced planning)
Back end changes
set to optional (see consideration below)(see consideration below)set to optionalset to optionalset to optionalset to optional(see consideration below)set to optionalset to optionalUpdate migrations to use the 3rd field where applicable:Vet Center: address3 from the source can now go to address_line3 rather than being combined with address2Vet Center - Outstation: address3 from the source can now go to address_line3 rather than being combined with address2Vet Center - Mobile Vet Center: address3 from the source can now go to address_line3 rather than being combined with address2NCA Facility: address3 from the source can now go to address_line3 rather than being combined with address2Update Post API to use address_line3.Consideration: Currently VAMC Facilities and VBA Facilities are migrating all 3 address lines into just 1 address line. Should this continue? If so then no additional things need to change. If not, then those content types will need adjustment as well.Consideration: Events live outside of Facilities. I don't think this would have too much affect on them in regards to the back end (just a config change), but there will need to be graphql and template changes on the frontend if the 3rd line is set as optional as indicated above. Otherwise the 3rd line should be set as hidden if it is determined to not be wanted/needed.Acceptance criteria
Update Front end changes and associated tests for Address module upgrade to version 2 #17193 with the findings of this ticketNo longer needed based on decision to hide line 3PM is tagged for communication with VHA DM and editorsNot needed based on decision to hide line 3The text was updated successfully, but these errors were encountered: