feat: add find and replace for passport variable via cli prompts #9

Mike-Heneghan · 2024-02-16T16:40:54Z

What

Reuse existing logic to build a script which takes in the current version of the passport variable and the new value it should be replaced with

Why

First use case would be migrating from using outbuildings to outbuilding
Opted to make this more abstract to cover potential similar use cases.

Testing

Local:

Simple flow testing
More complex flows (sequencing of portals with main services?)

Staging:

Simple flow testing
More complex flows (sequencing of portals with main services?)

Screenshots

outbuildings → outbuilding

outbuilding.shed → shed

Mike-Heneghan · 2024-02-16T16:42:40Z

findAndReplacePassportValue/helpers.js

+      const currentFn = nodeData["data"][`${passportKey}`]
+      const currentValueSegments = currentFn.split(".")
+      const matchIndexPosition = currentValueSegments.indexOf(currentPassportVariable)
+      if (matchIndexPosition !==-1) {


Due to this line the script will only ever replace the first instance of the exact match string.

I don't think there'd ever be cases of something like extend.outbuildings.shed.outbuildings but if that's not the case I can review the logic here.

If we look at real ProjectTypes, we can see that there are edge cases where the same word can appear twice - eg https://github.com/theopensystemslab/digital-planning-data-schemas/blob/main/types/enums/ProjectTypes.ts#L98

Is it actually necessary to split(".") the fn/val string here and then re-join, can we simply try a replaceAll() on the full passport string - which would then also give us more flexibility if we want to use this script ot replace full passport variables (any number characters/segments) rather than only a single segment of them?

Thanks for sharing the projectTypes and checking the assumption I'd made!

I'll have another look at this functionality. I had been using .replace the reason I was splitting was so I could do exact matches as I was worried about incorrectly replacing substrings so for something like alter → change incorrectly leadings toalteration.openings → changeation.openings

I'll have another look at replaceAll to see how whole sections can be updated 👍

Fixed here: 4bd0128

Oops sorry left this as a new comment rather than on this thread: #9 (comment)

jessicamcinchak

Looking good - a few small comments !

Happy to test locally again once those are resolved, then let's discuss which flows this should be run against on staging so that August & George can also test 👍

jessicamcinchak · 2024-02-19T09:29:14Z

findAndReplacePassportValue/helpers.js

+      const currentFn = nodeData["data"][`${passportKey}`]
+      const currentValueSegments = currentFn.split(".")
+      const matchIndexPosition = currentValueSegments.indexOf(currentPassportVariable)
+      if (matchIndexPosition !==-1) {


If we look at real ProjectTypes, we can see that there are edge cases where the same word can appear twice - eg https://github.com/theopensystemslab/digital-planning-data-schemas/blob/main/types/enums/ProjectTypes.ts#L98

Is it actually necessary to split(".") the fn/val string here and then re-join, can we simply try a replaceAll() on the full passport string - which would then also give us more flexibility if we want to use this script ot replace full passport variables (any number characters/segments) rather than only a single segment of them?

package.json

findAndReplacePassportValue/prompts.js

… than segments

jessicamcinchak · 2024-02-19T13:55:34Z

passportValue/helpers.js

+
+function replaceAllOccurrences(fullPassportValue, currentPassportVariable, newPassportVariable) {
+  const regex = new RegExp('\\b' + currentPassportVariable + '\\b', 'g');
+  return fullPassportValue.replace(regex, newPassportVariable);


Sorry - still not personally convinced this regex that protects against partial matches is better approach than plain string replaceAll() and I think is going to be ultimately limiting in use of this against "real" content examples!

In your example alter → change (but not alteration → changeation) that this regex protects against, I think you're overlooking the option to simply pass alter. as the find variable then replaceAll() as expected !!

In recent file naming, there were other examples of partial matches like councilTaxBill (find = councilTax) → councilBill (replace = council) that wouldn't be supported by this regex.

I'm maybe being too risk averse with this approach. As far as I understand the regex would approach would still allow all renaming but might require more runs as matches would need to be made explicitly councilTaxBill → councilBill vs the freedom / risk of partial match renames.

Passing in variables with . as in alter. is definitely something I hadn't considered and does overcome the issue I'd outlined although I guess it does require protecting against edge cases at the time of running the code and a deeper familiarity with the project types versus having that edge case protection in the code?

Although, I'm happy to defer to your judgement though as you're much more familiar with this process and the data.

Updated here: f073806

Thanks ! My theory on these is: always start quick & flexible based on known content examples, and introduce strictness as future use cases necessitate. Because since these scripts are only manually run in very controlled scenarios right now (eg not exposed to editors), we don't need to get hung up anticipating uses beyond the task at hand!

- As per: #9 (comment) - Allows more flexibility

jessicamcinchak

Thanks again for picking this one up! Happy for code to merge to main whenever you're ready.

As it's already a bit late in the afternoon & George isn't back until tomorrow, I'd advise running the outbuilding find&replace against staging tomorrow morning for final review from August & George (will get overwritten by nightly sync).

We'll want to run it against all flows that ask questions about project type (in addition to any parent services that nest those flows as external portals) - which I expect will be a quite long list I haven't started to compile yet. I can take over testing from here if helpful?

Once content team reviews staging & confirms automations are working as expected, I'll make the schema changes and run against production after hours later this week.

Mike-Heneghan · 2024-02-19T15:37:17Z

Thanks again for picking this one up! Happy for code to merge to main whenever you're ready.

As it's already a bit late in the afternoon & George isn't back until tomorrow, I'd advise running the outbuilding find&replace against staging tomorrow morning for final review from August & George (will get overwritten by nightly sync).

We'll want to run it against all flows that ask questions about project type (in addition to any parent services that nest those flows as external portals) - which I expect will be a quite long list I haven't started to compile yet. I can take over testing from here if helpful?

Once content team reviews staging & confirms automations are working as expected, I'll make the schema changes and run against production after hours later this week.

Thanks for the review @jessicamcinchak and as advised I'll try running outbuilding find&replace against staging tomorrow morning 👍

Yeah I'm still not sure how best to go about forming that exhaustive list of flows to update so if you don't mind picking it up from there that would be great.

jessicamcinchak · 2024-02-21T06:58:55Z

Find & replace "outbuildings" to "outbuilding" across project selection questions to fix flow automation and schema consistency

feat: add find and replace for passport variable via cli prompts

15865be

Mike-Heneghan self-assigned this Feb 16, 2024

Mike-Heneghan marked this pull request as draft February 16, 2024 16:41

Mike-Heneghan commented Feb 16, 2024

View reviewed changes

chore: add missing start script

769d674

Mike-Heneghan requested a review from a team February 16, 2024 17:39

jessicamcinchak reviewed Feb 19, 2024

View reviewed changes

Mike-Heneghan added 2 commits February 19, 2024 12:08

fix: update replace to match all occurrences and update values rather…

4bd0128

… than segments

chore: rename subdirectory

7fcb89b

Mike-Heneghan requested a review from jessicamcinchak February 19, 2024 12:41

jessicamcinchak reviewed Feb 19, 2024

View reviewed changes

refactor: update from regex replace to replaceAll

f073806

- As per: #9 (comment) - Allows more flexibility

Mike-Heneghan requested a review from jessicamcinchak February 19, 2024 14:34

jessicamcinchak approved these changes Feb 19, 2024

View reviewed changes

Mike-Heneghan marked this pull request as ready for review February 19, 2024 15:34

Mike-Heneghan merged commit 85b2d25 into main Feb 19, 2024
2 checks passed

Mike-Heneghan deleted the mh/add-script-for-updating-outbuildings-to-outbuilding branch February 19, 2024 15:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add find and replace for passport variable via cli prompts #9

feat: add find and replace for passport variable via cli prompts #9

Mike-Heneghan commented Feb 16, 2024 •

edited

Loading

Mike-Heneghan Feb 16, 2024

jessicamcinchak Feb 19, 2024

Mike-Heneghan Feb 19, 2024

Mike-Heneghan Feb 19, 2024

jessicamcinchak Feb 19, 2024

jessicamcinchak left a comment

jessicamcinchak Feb 19, 2024

jessicamcinchak Feb 19, 2024

Mike-Heneghan Feb 19, 2024

Mike-Heneghan Feb 19, 2024

jessicamcinchak Feb 19, 2024

jessicamcinchak left a comment •

edited

Loading

Mike-Heneghan commented Feb 19, 2024

jessicamcinchak commented Feb 21, 2024

feat: add find and replace for passport variable via cli prompts #9

feat: add find and replace for passport variable via cli prompts #9

Conversation

Mike-Heneghan commented Feb 16, 2024 • edited Loading

What

Why

Testing

Screenshots

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessicamcinchak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessicamcinchak left a comment • edited Loading

Choose a reason for hiding this comment

Mike-Heneghan commented Feb 19, 2024

jessicamcinchak commented Feb 21, 2024

Mike-Heneghan commented Feb 16, 2024 •

edited

Loading

jessicamcinchak left a comment •

edited

Loading