-
Notifications
You must be signed in to change notification settings - Fork 71
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
VACMS-14683: Updating service locations paragraph #15622
Conversation
I am rebasing the integration branch.
|
b83f4ce
to
c0d2253
Compare
d5fa239
to
bab50cc
Compare
beac8b6
to
ad1c1e7
Compare
I just touched off the migration on the Tugboat. |
This is likely going to have a conflict from the cherry-picked script-library that went out with the menu rearranging. I am not going to clear the conflict until after demo so we don't destroy any data in use by FE. |
ad1c1e7
to
fa2f54b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$batch_size = 25; | ||
$node_storage = get_node_storage(); | ||
$nids = array_slice($sandbox['items_to_process'], 0, $batch_size, TRUE); | ||
$facility_services = (empty($nids)) ? [] : $node_storage->loadMultiple(array_values($nids)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@omahane and @Becapa Because of the new state keeping logic in #17236 this will likely fall apart a bit because the state is tracking per item, not per batch.
The state does not know how to reference which batch of 25 was just finished and would not really be able to pick up where it left off. Also the output would likely be jibberish.
I suggest this be changed to remove the batch size and just grab the top item in the array. Something like this. (not tested)
$batch_size = 25; | |
$node_storage = get_node_storage(); | |
$nids = array_slice($sandbox['items_to_process'], 0, $batch_size, TRUE); | |
$facility_services = (empty($nids)) ? [] : $node_storage->loadMultiple(array_values($nids)); | |
$node_storage = get_node_storage(); | |
$nid_to_load = reset($sandbox['items_to_process']); | |
$facility_services = (empty($nid_to_load)) ? [] : $node_storage->loadMultiple(array_values($nid_to_load)); |
$node_storage = get_node_storage(); | ||
$nids = array_slice($sandbox['items_to_process'], 0, $batch_size, TRUE); | ||
$facility_services = (empty($nids)) ? [] : $node_storage->loadMultiple(array_values($nids)); | ||
foreach ($facility_services as $nid => $facility_service_node) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This foreach is no longer needed since there is only one node in $facility_services, but it would require a lot more shifting of logic to unwrap it than is probably necessary to get this working with the state keeping logic and way more than I can do in a PR suggestion.
As of today:
|
…dependabot version from main (#18042)
GitHub Workflows (.github/workflows/*.yml)Have you...
|
* Reverts unwanted composer.lock changes. * VACMS-14683: Reverts unwanted changes to default-branch-datadog-metrics.yml.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look great, nice work @omahane!
@department-of-veterans-affairs/platform-cms-devops-engineers FYI that this PR is ready for review if anyone would like to take a look. We will merge next Monday 5/13 for Monday daily deploy. After deploy, that'll be the trigger to run the related production script we've been talking to Grace / Tyler about. (cc @gracekretschmer-metrostar) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a lot of unfamiliar files and languages for me, but I looked through everything and didn't see any red flags or worrying signs. Glad that the merge conflicts get removed 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks!
Last minute ministrations around merging on Slack. |
Merge / launch planned for Monday 5/13. DO NOT MERGE PRIOR.
Description
Relates to #14683
This is a PR that is tracking the integration-service-location branch which contains work that can not go out until FE and CM have been done.
Note: To run the migration, but this in terminal:
drush scr ./scripts/content/VACMS-15559-migrate-service-location-from-node-to-paragraph.php
To build the front end:
Which version of content-build would you like to use?
BRANCH VACMS-16144-VAMC-ServiceLocationParagraphs
Which version of vets-website would you like to use?
BRANCH main
Testing done
Screenshots
QA steps
What needs to be checked to prove this works?
What needs to be checked to prove it didn't break any related things?
What variations of circumstances (users, actions, values) need to be checked?
As user uid with user_role
Definition of Done
Select Team for PR review
CMS Team
Public websites
Facilities
User support
Accelerated Publishing
Is this PR blocked by another PR?
DO NOT MERGE
Does this PR need review from a Product Owner
Needs PO review
CMS user-facing announcement
Is an announcement needed to let editors know of this change?