Skip to content

Conversation

@Zaimwa9
Copy link
Contributor

@Zaimwa9 Zaimwa9 commented Apr 9, 2025

Thanks for submitting a PR! Please check the boxes below:

  • I have added information to docs/ if required so people know about the feature!
  • I have filled in the "Changes" section below?
  • I have filled in the "How did you test this code" section below?
  • I have used a Conventional Commit title for this Pull Request

Changes

⚠️ Relies on #5325
Related to issue #5167

@tiagoapolo , following discussion with @kyle-ssg , we went for an in-between solution so that the required_for points to an organization while leaving the project implementation open for later. In this case it makes to relate the required_for field to the given organisation. Let me know your thoughts

  • Use organisation content_type in required_for
  • Pass organisation_id as object_id

How did you test this code?

https://www.loom.com/share/f17075de90744c64bc64554629ed3eca

  • Create custom field features in project settings
  • Switch projects to see them everywhere

Additional considerations

There are some points to keep in mind in the current metadata fields implementation. Just listing and depending on prio because quite some refacto.
As of Fields and Model fields are fetched with 2 different requests GET /fields and GET /metadata-model-fields then reconciliated in the frontend ( MetadataPage ) and it brings several problems long term:

  • The 2 endpoints have their own pagination. If ever we reach the limits, it will be very hard to synchronize the 2 lists and be sure there are no misses. So no clean pagination possible as of
  • It would be better to let the API handle the joins and the frontend just consumes them
  • 4 requests are made to create a single field create metadata field -> invalidated GET list -> create metadata model field -> invalidated GET list, it could be reduced imo
  • It's prone to error. It can happen that the metadata field is created but the second POST fails (metadata-model-field) and we end up with half of the data without a cleaning mechanism. Same I think we could have a single request creating both

@Zaimwa9 Zaimwa9 requested a review from a team as a code owner April 9, 2025 17:39
@Zaimwa9 Zaimwa9 requested review from tiagoapolo and removed request for a team April 9, 2025 17:39
@vercel
Copy link

vercel bot commented Apr 9, 2025

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

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 21, 2025 10:11am
flagsmith-frontend-preview ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 21, 2025 10:11am
flagsmith-frontend-staging ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 21, 2025 10:11am

@Zaimwa9 Zaimwa9 marked this pull request as draft April 9, 2025 17:39
@github-actions github-actions bot added the front-end Issue related to the React Front End Dashboard label Apr 9, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Apr 9, 2025

Docker builds report

Image Build Status Security report
ghcr.io/flagsmith/flagsmith-e2e:pr-5326 Finished ✅ Skipped
ghcr.io/flagsmith/flagsmith-api-test:pr-5326 Finished ✅ Skipped
ghcr.io/flagsmith/flagsmith-api:pr-5326 Finished ✅ Results
ghcr.io/flagsmith/flagsmith:pr-5326 Finished ✅ Results
ghcr.io/flagsmith/flagsmith-frontend:pr-5326 Finished ✅ Results

@github-actions
Copy link
Contributor

github-actions bot commented Apr 9, 2025

Uffizzi Preview deployment-62633 was deleted.

wood added 8 commits April 10, 2025 14:53
…b.com:Flagsmith/flagsmith into feat/metadata-requirements-object-id-optional
…b.com:Flagsmith/flagsmith into feat/remove-usage-of-required-for-object-id-in-metadata
…b.com:Flagsmith/flagsmith into feat/remove-usage-of-required-for-object-id-in-metadata
…b.com:Flagsmith/flagsmith into feat/remove-usage-of-required-for-object-id-in-metadata
@Zaimwa9 Zaimwa9 changed the title Feat: (wip) remove-usage-of-required-for-object-id-in-metadata Feat: changed-required-for-to-use-organisation-content-type-and-object-id Apr 10, 2025
@Zaimwa9 Zaimwa9 marked this pull request as ready for review April 10, 2025 13:23
@Zaimwa9 Zaimwa9 changed the title Feat: changed-required-for-to-use-organisation-content-type-and-object-id Feat: MetadataModelRequirement uses organisation ID as objectID Apr 10, 2025
@tiagoapolo tiagoapolo changed the title Feat: MetadataModelRequirement uses organisation ID as objectID feat: MetadataModelRequirement uses organisation ID as objectID Apr 10, 2025
@tiagoapolo
Copy link
Contributor

Great! 🎉

Copy link
Contributor

@tiagoapolo tiagoapolo left a comment

Choose a reason for hiding this comment

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

I left a suggestion but overall looks good to me and I'm aligned with that approach.

@github-actions github-actions bot added the feature New feature or request label Apr 11, 2025
@Zaimwa9 Zaimwa9 changed the title feat: MetadataModelRequirement uses organisation ID as objectID feat: (🔴 blocked by #5325) MetadataModelRequirement uses organisation ID as objectID Apr 11, 2025
@Zaimwa9 Zaimwa9 changed the title feat: (🔴 blocked by #5325) MetadataModelRequirement uses organisation ID as objectID feat: MetadataModelRequirement uses organisation ID as objectID (🔴 blocked by #5325) Apr 11, 2025
Base automatically changed from feat/metadata-requirements-object-id-optional to main April 17, 2025 09:20
@Zaimwa9 Zaimwa9 requested a review from a team as a code owner April 17, 2025 09:20
@Zaimwa9 Zaimwa9 requested review from gagantrivedi and removed request for a team April 17, 2025 09:20
@github-actions github-actions bot added api Issue related to the REST API feature New feature or request and removed feature New feature or request labels Apr 21, 2025
@Zaimwa9 Zaimwa9 changed the title feat: MetadataModelRequirement uses organisation ID as objectID (🔴 blocked by #5325) feat: MetadataModelRequirement uses organisation ID as objectID (✅ relies on #5325 - deployed) Apr 21, 2025
@codecov
Copy link

codecov bot commented Apr 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.61%. Comparing base (de92415) to head (dc7b8e1).
Report is 10 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5326   +/-   ##
=======================================
  Coverage   97.61%   97.61%           
=======================================
  Files        1237     1237           
  Lines       42971    42972    +1     
=======================================
+ Hits        41948    41949    +1     
  Misses       1023     1023           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Zaimwa9 Zaimwa9 merged commit 8dc92c3 into main Apr 23, 2025
37 checks passed
@Zaimwa9 Zaimwa9 deleted the feat/remove-usage-of-required-for-object-id-in-metadata branch April 23, 2025 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api Issue related to the REST API feature New feature or request front-end Issue related to the React Front End Dashboard

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Project ID should not be required when creating/updating /metadata-model-fields

3 participants