Skip to content
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

Fix getChangeHistory #805

Merged
merged 1 commit into from
Apr 29, 2023
Merged

Fix getChangeHistory #805

merged 1 commit into from
Apr 29, 2023

Conversation

zichongkao
Copy link
Contributor

@zichongkao zichongkao commented Apr 29, 2023

In response to breakage reported here: #782.

In working on this, I considered whether we should be showing Organizations history in the general changelog. I feel it could go either way:

  • If we think of the changelog as helping folks keep tabs on user-executed changes so that we can fix them if there is abuse, then Organization changes shouldn't be there because these are done by LCO admins whom we trust more and are less at risk of abuse.
  • If we think of the changelog as showing folks what's going on the site that might be of public interest, then since organizations are highly visible, it makes sense to show changes to them.

In the end, I figured we can just expand functionality to show a change (including those for organizations) if it exists. If we want to remove organization changes, the best place is probably at the GraphQL query layer, by adding a filter.

The current filter doesn't support kind. The problem is that a changelog of a certain kind can involve documents of multiple kinds. Eg. add climb (changelog kind == 'climbs') involves inserting a climb (document kind == 'climbs') and updating the parent area (document kind = 'areas'). It's not clear whether we should include the parent area document change when filtering for climbing changelogs. So there's some nuance here to think about.

For context, kind is stabilized on the backend in this OpenBeta/openbeta-graphql#277.

@vercel
Copy link

vercel bot commented Apr 29, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
open-tacos ✅ Ready (Inspect) Visit Preview Apr 29, 2023 0:51am

@@ -51,7 +52,7 @@ const ChangesetRow = ({ changeset }: ChangsetRowProps): JSX.Element => {
}

const ClimbChange = ({ changeId, fullDocument, updateDescription, dbOp }: ChangeType): JSX.Element | null => {
if ((fullDocument as ClimbType)?.name == null) {
if (fullDocument.__typeName !== DocumentTypeName.Climb) {
Copy link
Contributor Author

@zichongkao zichongkao Apr 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More robust to use __typename than unique document fields: https://www.apollographql.com/docs/apollo-server/schema/unions-interfaces/#querying-a-union

@zichongkao
Copy link
Contributor Author

Note that builds will keep failing until newest openbeta-graphql changes are deployed to staging. Otherwise during build, we prepare static props which requires calling the staging API and that fails because the old code doesn't expect Organization document changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants