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

Add metadata to data being sent to API endpoint in json dump registration backend #5053

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

viktorvanwijk
Copy link
Contributor

@viktorvanwijk viktorvanwijk commented Jan 28, 2025

Closes #5012

Changes

Add metadata to data being sent to API endpoint in JSON dump registration backend

Checklist

Check off the items that are completed or not relevant.

  • Impact on features

    • Checked copying a form
    • Checked import/export of a form
    • Config checks in the configuration overview admin page
    • Problem detection in the admin email digest is handled
  • Release management

    • I have labelled the PR as "needs-backport" accordingly
  • I have updated the translations assets (you do NOT need to provide translations)

    • Ran ./bin/makemessages_js.sh
    • Ran ./bin/compilemessages_js.sh
  • Dockerfile/scripts

    • Updated the Dockerfile with the necessary scripts from the ./bin folder
  • Commit hygiene

    • Commit messages refer to the relevant Github issue
    • Commit messages explain the "why" of change, not the how

@viktorvanwijk viktorvanwijk force-pushed the feature/5012-add-metadata-to-data-being-sent-to-api-endpoint-in-json-dump-registration-backend branch 4 times, most recently from 85cfc56 to 427717f Compare January 30, 2025 15:39
@viktorvanwijk viktorvanwijk force-pushed the feature/5012-add-metadata-to-data-being-sent-to-api-endpoint-in-json-dump-registration-backend branch 2 times, most recently from 047a0c9 to 54efef5 Compare January 30, 2025 15:55
Copy link

codecov bot commented Jan 30, 2025

Codecov Report

Attention: Patch coverage is 98.38710% with 1 line in your changes missing coverage. Please review.

Project coverage is 96.72%. Comparing base (bc1cc34) to head (5a5e6a8).
Report is 18 commits behind head on master.

Files with missing lines Patch % Lines
...penforms/registrations/contrib/json_dump/plugin.py 93.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5053      +/-   ##
==========================================
+ Coverage   96.71%   96.72%   +0.01%     
==========================================
  Files         770      771       +1     
  Lines       26489    26573      +84     
  Branches     3453     3460       +7     
==========================================
+ Hits        25618    25704      +86     
  Misses        607      607              
+ Partials      264      262       -2     

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

@viktorvanwijk viktorvanwijk force-pushed the feature/5012-add-metadata-to-data-being-sent-to-api-endpoint-in-json-dump-registration-backend branch from 54efef5 to 8f487bf Compare January 30, 2025 16:14
@viktorvanwijk viktorvanwijk marked this pull request as ready for review January 31, 2025 08:20
@viktorvanwijk
Copy link
Contributor Author

viktorvanwijk commented Jan 31, 2025

Few design considerations:

Tried making the JSONDumpOptionsSerializer.required_metadata_variables field read_only, but then it won't be available in validated_data of the serializer, which is passed to the JSONDumpRegistration.register_submission. Thought about overwriting validated_data and adding the required_metadata_variables manually, but didn't seem like the right thing to do. Or adding a non-field constant for the default values of required_metadata_variables, so it can be accessed easily from the plugin without instantiating the serializer, but not sure if there are any best practises for that.

To show all metadata variables (additional and required) in the metadata selection box, and grey out all variables which are required by default, thought about adding the registration variables to the VariablesSelection component. But, because they are just static variables, they would all be added to this group in the drop-down menu. And afaik we cannot guarantee unique keys between regular static variables and registration variables (or registration variables of different plugins as well). So I would have to redo the grouping of variables in this component, and wasn't sure it would be worth it, or even desired behaviour.

@viktorvanwijk viktorvanwijk force-pushed the feature/5012-add-metadata-to-data-being-sent-to-api-endpoint-in-json-dump-registration-backend branch from 8f487bf to 205d6aa Compare January 31, 2025 10:20
Also add support for an extra static variables registry in generate_json_schema
If there is no authentication, the value can be an empty string
If there is no content (in case of an HTTP 204 response for example), converting the response to json results in an error, so we just return an empty string
@viktorvanwijk viktorvanwijk force-pushed the feature/5012-add-metadata-to-data-being-sent-to-api-endpoint-in-json-dump-registration-backend branch from 205d6aa to 5a5e6a8 Compare January 31, 2025 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add metadata to data being sent to API endpoint in JSON dump registration backend
1 participant