-
-
Notifications
You must be signed in to change notification settings - Fork 134
Update of users representatives/senators with court 194 #1755
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
base: main
Are you sure you want to change the base?
Update of users representatives/senators with court 194 #1755
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Love the first pass here - sorry for the late review!
The full blockers here are the double-senator update bug and ensuring that we only update profiles that actually need to be updated.
import { currentGeneralCourt } from "../../functions/src/shared" | ||
|
||
export const script: Script = async ({ db, args }) => { | ||
console.log(`Update General Court ${currentGeneralCourt} in progress...`) |
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.
Love the logging here - it makes this super easy to follow!
const writer = db.bulkWriter() | ||
|
||
console.log("Fetching all profiles ...") | ||
const users = await db.collection("profiles").get() |
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.
Just for posterity - this is likely fine because there are currently only like ~500 profiles, which is roughly the default bulk writer batch size anyway..
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.
Good to know!
import { Script } from "./types" | ||
import { currentGeneralCourt } from "../../functions/src/shared" | ||
|
||
export const script: Script = async ({ db, args }) => { |
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.
One nit: could we add a dryRun
arg here, and only actually make the updates if it's set to true
? This would make it easier for us to first run this script without making any changes just to see that the proposed changes look correct.
There's other examples of us using args in other scripts, like here:
const Args = Record({ |
console.log(`${users.size} profiles fetched`) | ||
|
||
for (const user of users.docs) { | ||
let data = user.data() |
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.
For convenience, you could likely use pre-existing types here, e.g.:
let data = user.data() as Profile
const currentSenator = data // This will be a ProfileMember from "components/db/profile/types.ts"
|
||
if (data.hasOwnProperty("senator")) { | ||
const senator = data.senator | ||
if (courtMembersMap.has(senator.district)) { |
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.
I don't feel that this is precisely the right comparison - we only want to update a Profile if the profile has an existing senator and/or representative who DOESN'T exist in the new general court.
We expect the district to be the same for both the court 193 members and court 194 members, so we should likely be checking if the name in the courtMembersMap
for the profile's districts matches the name on the profile's rep - if they match, we don't need to update this member on this profile.
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.
As a side-note, it'd be nice to track the counts of users we're updating/not updating to log out at the end (e.g. something like numUsersWithoutSenator
/numUsersWithUpToDateSenator
/numUsersUpdated
) - that would be useful for the dry run to get a sense of how many updates to expect.
`User ${data.displayName} has senator ${senator.name} ${senator.id} that is in the general court 194` | ||
) | ||
const senatorCurrentGC = courtMembersMap.get(senator.district) | ||
data.senator.id = senatorCurrentGC.id |
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.
I feel like it's better practice not to overwrite the data
object here - we should create a new object to represent the desired update data - e.g. something like:
const senator = {...data.senator}
if (shouldUpdate) {
senator = {
...senator,
id: senatorCurrentGC.id,
name: senatorCurrentGC.content["Name"]
}
writer.update(user.ref, { senator } )
}
with UNKNOW district ${senator.district} in the general court 194`) | ||
} | ||
} else { | ||
console.log("WARNING: no senator found!") |
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.
I don't feel this needs to be a warning per se (we expect that not all users will have set their senators/represenatives on their profile) - but it might be handy to include the user's id here in the log message (e.g. No senator set on profile ${user.id}!
)
console.log("WARNING: no senator found!") | ||
} | ||
|
||
if (data.hasOwnProperty("representative")) { |
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.
It might be helpful to extract a helper function for this block, given that the senator
and representative
blocks here have near identical logic. e.g. something like const updateProfileMember = (profileRef, memberType, currentMemberMap, bulkWriter) => { // doStuff()}
- where memberType
is "senator" | "represenative"
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.
All comments I made on the senator
block also apply here, so this might make it easier to make changes in just one place.
) | ||
|
||
writer.update(db.doc(`/profiles/${user.id}`), { | ||
senator: data.senator |
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 should be representative
- we're currently updating senator
twice, so this will both fail to update representative
and might result in some users' senator
being replaced with their representative
, depending on the order in which the updates actually occur.
Summary
Update the users representatives and senators using general court 194 recently released.
Screenshots
Here is an example of the output produced using Firebase locally:
`yarn firebase-admin -e local run-script updateGeneralCourt.ts
yarn run v1.22.22
$ ts-node -P tsconfig.script.json scripts/firebase-admin --swc -e local run-script updateGeneralCourt.ts
{"severity":"WARNING","message":"Warning, estimating Firebase Config based on GCLOUD_PROJECT. Initializing firebase-admin may fail"}
Update General Court 194 in progress...
(node:11022) [DEP0040] DeprecationWarning: The
punycode
module is deprecated. Please use a userland alternative instead.(Use
node --trace-deprecation ...
to show where the warning was created)65 members fetched
Fetching all profiles ...
11 profiles fetched
User info: 05lcy6YQrPvPRvVPlZFbCA0O81TF
WARNING: no senator found!
WARNING: no representative found!
User info: 0BvO7rSlFjRVHuLfd7RlHRYg2DN2
User undefined has MJB0 Michael J. Barrett
with UNKNOW district Third Middlesex in the general court 194
User undefined has TMS1 Thomas M. Stanley
with UNKNOW district 9th Middlesexin the general court 194
User info: CDxcKBZ6BjgetS4C2yuXbB5wgFC2
User undefined has senator Brendan P. Crighton BPC0 that is in the general court 194
Updating Brendan P. Crighton BPC0
User undefined has representative Bud L. Williams BLW1 that is in the general court 194
Updating representative Bud L. Williams BLW1
User info: Nh6InFWoTvgFMPQwhsLSTzeuxvL2
User undefined has WNB0 William N. Brownsberger
with UNKNOW district Second Suffolk and Middlesex in the general court 194
User undefined has KGH1 Kevin G. Honan
with UNKNOW district 17th Suffolkin the general court 194
User info: Ovr678WJoHrtsQ6TnQqQRmaXufon
WARNING: no senator found!
WARNING: no representative found!
User info: PbB7tQy18F9bQXfI3AonVmFfQU1B
WARNING: no senator found!
WARNING: no representative found!
User info: VBN6dg70PGsupNvayJ2TPHPgydHz
WARNING: no senator found!
WARNING: no representative found!
User info: d6ZFKTVH8i4hglyv42wz8QDaKKxw
WARNING: no senator found!
WARNING: no representative found!
User info: jCPFwLOo45a18GjP2qLYhXJLjKa2
User undefined has AGH0 Adam G. Hinds
with UNKNOW district Berkshire, Hampshire, Franklin and Hampden in the general court 194
WARNING: no representative found!
User info: jLPye8OfwlzAEGO6h10hs5mmVIDE
WARNING: no senator found!
WARNING: no representative found!
User info: ynu4qGC93gUdZgu5kpHbXNQS6Co1
User undefined has WNB0 William N. Brownsberger
with UNKNOW district Second Suffolk and Middlesex in the general court 194
User undefined has KGH1 Kevin G. Honan
with UNKNOW district 17th Suffolkin the general court 194
Updated 11 documents
✨ Done in 4.51s.`
Known issues
The issue was that the General Court 194 had just been released and we needed to update all the known representatives and senators from users.
Steps to test/reproduce
$ yarn dev:up
and check that the Firebase Emulator is available at http://127.0.0.1:3010.$yarn firebase-admin -e local run-script runScrapers.ts
to scrape the general court 194. The Firebase Emulator Database should be updated at http://127.0.0.1:3010/firestore/data/generalCourts/194/yarn firebase-admin -e local run-script updateGeneralCourt.ts
, the output should as the ones in the # Screenshots above.