Show eCapris status syncing updates in project activity log#1618
Conversation
| ```bash | ||
| ./hasura_cluster start_only | ||
| ./run_migrations | ||
| hasura migrate apply |
There was a problem hiding this comment.
if you try ./run_migrations
zsh: no such file or directory: ./run_migrations`
if you try ./hasura-cluster run_migrations
./hasura-cluster: line 376: run_migrations: command not found
so I updated the README to use the basic hasura cli command
| // check truthiness to prevent rendering String(null) as "null" | ||
| !!changeData.new[changedField] && | ||
| String(changeData.new[changedField]).length > 0 | ||
| String(changeData.new[changedField]).length > 0 |
| }, | ||
| project_name_secondary: { | ||
| label: "secondary project name" | ||
| label: "secondary project name", |
There was a problem hiding this comment.
the rest of the updates in this file are just autoformatting...
| const changedField = change.description[0].field; | ||
| // Get the changed fields from the description | ||
| const changedFields = change.description[0]?.fields || []; | ||
| const changedField = changedFields[0]; |
There was a problem hiding this comment.
This got a little tricky. Previously, we've been assuming there is only one changed field, but the current behavior with eCAPRIS updates is that you can have both should_sync_ecapris_statusesand ecapris_subproject_id together as one activity.
So I tried to account for this by letting changedFields be an array but not changing everything and letting changedField assume the first entry in the array.
This felt like the best way to not rock the boat but create an on-ramp to evaluate if the changedFields array contains one or both of the ecapris activity types.
There was a problem hiding this comment.
I agree that this feels like a good way to avoid a large refactor. Thanks for handling it and good to know. 🙏
| const newData = outputEvent.record_data.event.data.new; | ||
| const oldData = outputEvent.record_data.event.data.old; | ||
| let changedField = ""; | ||
| let changedFields = []; |
There was a problem hiding this comment.
Continuing the theme here. Updating changedField from one string to changedFields as an array. This is probably where my code could use the most skeptical eye that this change doesn't effect other patterns.
There was a problem hiding this comment.
Thanks for flagging! I looked many different production project activity logs with this update, and I didn't find anything that looked out of order.
| changeIcon, | ||
| changeText: [ | ||
| { | ||
| text: "Updated eCAPRIS subproject statuses sync for ", |
There was a problem hiding this comment.
i almost was wanting this to say "Updated eCAPRIS subproject ID from ___ to ___", similar to the message for when you add or delete a subproject ID. this message was kind of reading as if it was the sync status that changed
There was a problem hiding this comment.
Yeah, I think you're right. If we wanted to be extra verbose, maybe "Updated eCAPRIS subproject ID and statuses sync from ___ to ___". Since technically the sync status didn't change but it's updated and activated by default with the new subproject ID.
There was a problem hiding this comment.
I am all about this change. 🙏 Thanks y'all and could you update to what you suggested @mateoclarke? I can also update later if needed.
There was a problem hiding this comment.
just pushed the update!
roseeichelmann
left a comment
There was a problem hiding this comment.
I like the way you handled this with adding the changedFields array. I have one comment on maybe changing the wording of one of the activity log messages, but other than that this looks and works great to me!!
mddilley
left a comment
There was a problem hiding this comment.
Looks awesome! I tested all combinations of id and sync updates on my local instance, and I see the activity log reflect the changes. Cool to see this code extended too! 🚀
Love the improved wording that you and Rose discussed. 🚢
| const changedField = change.description[0].field; | ||
| // Get the changed fields from the description | ||
| const changedFields = change.description[0]?.fields || []; | ||
| const changedField = changedFields[0]; |
There was a problem hiding this comment.
I agree that this feels like a good way to avoid a large refactor. Thanks for handling it and good to know. 🙏
| changeIcon, | ||
| changeText: [ | ||
| { | ||
| text: "Updated eCAPRIS subproject statuses sync for ", |
There was a problem hiding this comment.
I am all about this change. 🙏 Thanks y'all and could you update to what you suggested @mateoclarke? I can also update later if needed.
| const newData = outputEvent.record_data.event.data.new; | ||
| const oldData = outputEvent.record_data.event.data.old; | ||
| let changedField = ""; | ||
| let changedFields = []; |
There was a problem hiding this comment.
Thanks for flagging! I looked many different production project activity logs with this update, and I didn't find anything that looked out of order.
|
Noting that I retested the latest commit, and it looks great. 🚢 @mateoclarke Thanks for noting the need to refresh the activity log after making project updates. I dug up cityofaustin/atd-data-tech#15828 which was created to address. |
johnclary
left a comment
There was a problem hiding this comment.
Awesome work—these activity log messages look really excellent. So fancy is the Moped activity log 😍
chiaberry
left a comment
There was a problem hiding this comment.
I really like how you were able to address multiple fields being updated without it cascading down to be a huge refactor. Its been a long time since I've really looked at the activity log code and I am trying to remember if there are other activities that possibly update more than one field at a time? future work!
Handle eCAPRIS sync activity in log when there is no eCAPRIS subproject id
Associated issues
Closes cityofaustin/atd-data-tech#22431
Note: It's not in scope here, but I'd recommend a follow-up issue to make it so that Activity Log data automatically refreshes instead of having to manually refresh to see updates in the tab. cc/ @mddilley
Testing
Will have to test locally since this is a branch off a branch.
Steps to test:
Ship list
[ ] Product manager added to QA test script if applicable