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

4723 org link preview #212

Merged
merged 3 commits into from
Sep 12, 2024
Merged

4723 org link preview #212

merged 3 commits into from
Sep 12, 2024

Conversation

robert-bryson
Copy link
Contributor

Related to GSA/data.gov#4723

@robert-bryson
Copy link
Contributor Author

This is working correctly as is. Perhaps the cloudfront cache hadn't updated when looking before.

Copy link
Contributor

@btylerburton btylerburton left a comment

Choose a reason for hiding this comment

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

LGTM. I've got a couple non-blocking questions:

  1. Is accessing properties of c object safe without a default? (i.e. c.pkg_dict or c.group_dict)
  2. Is the outputted content what you want? Is it worth writing a test to prove it?

@FuhuXia
Copy link
Member

FuhuXia commented Sep 12, 2024

This is working correctly as is. Perhaps the cloudfront cache hadn't updated when looking before.

You means it is already deployed to prod?

@robert-bryson
Copy link
Contributor Author

This is working correctly as is. Perhaps the cloudfront cache hadn't updated when looking before.

You means it is already deployed to prod?

No, it is on dev.
image
https://catalog-dev.data.gov/dataset/digital-flood-insurance-rate-map-database-newport-county-rhode-island

It could've been something else that caused the delay. I did not investigate.

@robert-bryson
Copy link
Contributor Author

LGTM. I've got a couple non-blocking questions:

  1. Is accessing properties of c object safe without a default? (i.e. c.pkg_dict or c.group_dict)

I'm not sure. The defaults set in lines 3-7 are to provide defaults in the case that a page is not a resource (e.g., has a c.pkg_dict) or an organization (e.g., has a c.group_dict), not explicitly for safety. That is, I suppose, a handy knock-on effect. I refactored to this approach to help avoid repeating code and improve legibility.

  1. Is the outputted content what you want? Is it worth writing a test to prove it?

Yes, it does appear correct. There is no good reason to not write a test, I suppose. It would be a best practice, certainly. I didn't partially out of speed and partially that if there is an issue with the pkg or org objects the defaults will fill in.

@robert-bryson robert-bryson merged commit 9750cf6 into main Sep 12, 2024
4 checks passed
@robert-bryson robert-bryson deleted the 4723_org-link-preview branch September 12, 2024 16:04
@robert-bryson
Copy link
Contributor Author

Thanks for the review, @btylerburton and @FuhuXia!

@FuhuXia
Copy link
Member

FuhuXia commented Sep 12, 2024

Test Org in development showing the right title.
you can use https://catalog-dev.data.gov/organization/test-org?abc to bust the cache.
image

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.

3 participants