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

[#4396] Objects API prefill plugin #4620

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

Conversation

vaszig
Copy link
Contributor

@vaszig vaszig commented Aug 26, 2024

Closes #4396 partially

Changes

  • Added necessary model field prefill_options to the FormVariable model (for storing all the needed configuration) and updated constraints according to new field
  • Moved public prefill functions to prefill.service
  • Added prefill functionality for ObjectsAPI

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
  • Commit hygiene

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

Copy link

codecov bot commented Aug 26, 2024

Codecov Report

Attention: Patch coverage is 92.41071% with 17 lines in your changes missing coverage. Please review.

Project coverage is 96.54%. Comparing base (751991d) to head (2890501).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...c/openforms/forms/api/serializers/form_variable.py 55.55% 7 Missing and 1 partial ⚠️
src/openforms/prefill/service.py 87.93% 3 Missing and 4 partials ⚠️
src/openforms/prefill/sources/user_defined.py 92.30% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4620      +/-   ##
==========================================
- Coverage   96.56%   96.54%   -0.03%     
==========================================
  Files         746      750       +4     
  Lines       25210    25340     +130     
  Branches     3322     3341      +19     
==========================================
+ Hits        24345    24465     +120     
- Misses        602      611       +9     
- Partials      263      264       +1     

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

@vaszig vaszig marked this pull request as draft August 26, 2024 11:41
@vaszig vaszig force-pushed the task/4396-objectsapi-prefill-implement-plugin branch from 66ed5a5 to 482bc20 Compare August 27, 2024 14:30
@vaszig vaszig force-pushed the task/4396-objectsapi-prefill-views-and-urls branch from 0ee299f to 2b992bf Compare August 27, 2024 14:36
@vaszig vaszig force-pushed the task/4396-objectsapi-prefill-implement-plugin branch 4 times, most recently from 2b6a3f0 to 8eb07c9 Compare August 28, 2024 08:51
@vaszig vaszig force-pushed the task/4396-objectsapi-prefill-views-and-urls branch from 2b992bf to 50c9377 Compare August 28, 2024 10:20
@vaszig vaszig force-pushed the task/4396-objectsapi-prefill-implement-plugin branch from 8eb07c9 to 0d9f00c Compare August 28, 2024 10:23
@vaszig vaszig force-pushed the task/4396-objectsapi-prefill-views-and-urls branch from 50c9377 to 70f0382 Compare August 28, 2024 12:44
@vaszig vaszig force-pushed the task/4396-objectsapi-prefill-implement-plugin branch from 0d9f00c to b7e03db Compare August 28, 2024 13:02
@vaszig vaszig requested a review from stevenbal August 28, 2024 13:21
@vaszig vaszig marked this pull request as ready for review August 28, 2024 13:21
@vaszig vaszig force-pushed the task/4396-objectsapi-prefill-implement-plugin branch 2 times, most recently from 7ac1670 to 71cc7e8 Compare September 2, 2024 06:12
@vaszig vaszig force-pushed the task/4396-objectsapi-prefill-views-and-urls branch from 70f0382 to 0dfe280 Compare September 6, 2024 09:31
@vaszig vaszig marked this pull request as draft September 6, 2024 12:39
@vaszig vaszig removed the request for review from stevenbal September 6, 2024 12:39
@vaszig vaszig force-pushed the task/4396-objectsapi-prefill-views-and-urls branch from 7b222b9 to 59304cb Compare September 13, 2024 13:41
Base automatically changed from task/4396-objectsapi-prefill-views-and-urls to master September 16, 2024 12:21
@vaszig vaszig force-pushed the task/4396-objectsapi-prefill-implement-plugin branch 3 times, most recently from a7c3df4 to e60e61c Compare September 23, 2024 14:43
@vaszig vaszig force-pushed the task/4396-objectsapi-prefill-implement-plugin branch 3 times, most recently from 0b5b2cd to 1a4c02c Compare October 2, 2024 07:18
@vaszig vaszig marked this pull request as ready for review October 2, 2024 07:19
Comment on lines +31 to +34
return patch(
"openforms.prefill.service.inject_prefill",
new=MagicMock,
)
Copy link
Member

Choose a reason for hiding this comment

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

isn't this broken now because of the local import?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see a problem here in the tests and if I remove this the tests are failing.

@vaszig vaszig force-pushed the task/4396-objectsapi-prefill-implement-plugin branch 2 times, most recently from 050232d to cf2bb25 Compare October 10, 2024 14:55
@sergei-maertens sergei-maertens force-pushed the task/4396-objectsapi-prefill-implement-plugin branch from cf2bb25 to 13938ae Compare October 11, 2024 09:21
src/openforms/forms/models/form_variable.py Show resolved Hide resolved
Comment on lines 222 to 227
| (
~EMPTY_PREFILL_PLUGIN
& EMPTY_PREFILL_ATTRIBUTE
& EMPTY_PREFILL_OPTIONS
& ~USER_DEFINED
)
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this one -

  • ~USER_DEFINED implies COMPONENT (since there are only two options)
  • ~EMPTY_PREFILL_PLUGIN implies that a plugin is selected.

this is the behaviour of the master branch - and here the check constraint says that then also the attribute must be set: ~EMPTY_PREFILL_ATTRIBUTE, but the code here allows/requires an empty attribute 🤔

shouldn't this combination be disallowed entirely? You then end up with the following options:

  • no prefill configured at all (no plugin selected -> no attribute or options may be set)
  • "previous" prefill configuration (plugin selected, attribute selected -> prefill options must be empty), can be used for both components and user defined vars
  • "new" prefill configuration (plugin selected, options set -> attribute may not be set, source must be user defined)

I think this union member is not needed then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will remain (if I am not mistaken) because of the comment below
#4620 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Unresolving this as a reminder so that we can discuss this at the end of the day to make sure I understand everything correctly!

src/openforms/forms/models/form_variable.py Show resolved Hide resolved
src/openforms/forms/models/form_variable.py Outdated Show resolved Hide resolved
src/openforms/forms/api/serializers/form_variable.py Outdated Show resolved Hide resolved
src/openforms/forms/api/serializers/form_variable.py Outdated Show resolved Hide resolved
src/openforms/prefill/base.py Outdated Show resolved Hide resolved
src/openforms/prefill/base.py Outdated Show resolved Hide resolved
src/openforms/prefill/contrib/objects_api/plugin.py Outdated Show resolved Hide resolved
@vaszig vaszig force-pushed the task/4396-objectsapi-prefill-implement-plugin branch 2 times, most recently from 3aa0da1 to 96023e0 Compare October 18, 2024 10:15
@vaszig vaszig force-pushed the task/4396-objectsapi-prefill-implement-plugin branch from 96023e0 to 25a6f91 Compare October 18, 2024 10:27
@vaszig vaszig force-pushed the task/4396-objectsapi-prefill-implement-plugin branch from 25a6f91 to 2890501 Compare October 18, 2024 12:29
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.

Create a prefill plugin to prefill with values from an external reference (initially only for Objects API)
2 participants