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 logic for the new registration (static) variables #3984

Merged
merged 11 commits into from
Mar 14, 2024

Conversation

Viicos
Copy link
Contributor

@Viicos Viicos commented Mar 7, 2024

After a lot of thinking about the technical implementation:

  • Having all registration static variables registered under the same registry is not reliable, as I expect registration plugins to provide similar (if not identical) static variables, and duplicate identifiers would fail.
  • To overcome this issue, each plugin can provide their own registry.
  • get_static_variables was adapted so that it can iterate over a provided registry, instead of the default one for vanilla static variables.
  • A bit unrelated but can be reviewed in separate commits: an endpoint returning the available registration variables was added. It is designed mainly as a backend for frontend endpoint.

This preserves backward compatibility, as get_static_variables([, submission=...]) will still use the default registry. It also works well this way because get_static_variables is currently used in:

  • jsonlogic trigger validation: ✅
    # Check if the trigger references a static variable
    needle_bits = needle.split(".")
    for variable in get_static_variables():

    registration static variables shouldn't be referenced.
  • Validate form variables are unique: ⚠️
    if item["key"] in static_data_keys:
    errors[f"{index}.key"].append(

    Ideally registration variables should be taken into account as well. Not sure how common a name collision between a form variable and a registration variable would be. If we want to validate this, it might get tricky as every registration backend should be taken into account. So perhaps a new include_registration boolean option to get_static_variables could be added? (Issue is it is conflicting with the variables_registry argument).
  • get_variables_for_context: ✅
    formio_data = FormioData(
    **{
    variable.key: variable.initial_value
    for variable in get_static_variables(submission=submission)
    },

    get_variables_for_context is only used for the Objects API (where it makes sense to not include reg. variables, those are added manually in the context by accessing ObjectsAPIRegistrationData) and the email backend (not sure about this one, probably doesn't make sense to have registration variables for this plugin anyway?).
  • static variables endpoint: ✅
    class StaticFormVariablesView(ListMixin, APIView):
    authentication_classes = (authentication.SessionAuthentication,)
    permission_classes = (permissions.IsAdminUser,)
    serializer_class = FormVariableSerializer
    def get_objects(self):
    return get_static_variables()

    As expected, reg. variables are in a separate endpoint.

For tests, I'm planning on testing I tested the ObjectsAPIRegistrationData creation, in particular how it behaves when I get an API error when trying to create documents. Not sure where to put these, ideally I don't want to touch the big 1k lines ObjectsAPIBackendTests test class, although it might require some refactor?

Added registration variables will be tested in #3982, when the v2 registration will be tested.

Viicos added 2 commits March 7, 2024 17:28
Drop the dataclass, makes it confusing with field excluded
from init, and the fact that the identifier needs to be
specified again

Use class vars instead

Make use of newly added type alias
@Viicos Viicos changed the title Feature/3688 registration variables Add logic for the new registration (static) variables Mar 7, 2024
Copy link

codecov bot commented Mar 8, 2024

Codecov Report

Attention: Patch coverage is 86.41975% with 22 lines in your changes are missing coverage. Please review.

Project coverage is 96.02%. Comparing base (bed8118) to head (86401a6).
Report is 28 commits behind head on master.

Files Patch % Lines
...ions/contrib/objects_api/registration_variables.py 68.96% 18 Missing ⚠️
...ons/contrib/objects_api/submission_registration.py 96.22% 0 Missing and 2 partials ⚠️
src/openforms/registrations/base.py 75.00% 1 Missing ⚠️
...nforms/registrations/contrib/objects_api/plugin.py 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3984      +/-   ##
==========================================
- Coverage   96.11%   96.02%   -0.10%     
==========================================
  Files         724      726       +2     
  Lines       22625    22753     +128     
  Branches     2607     2620      +13     
==========================================
+ Hits        21747    21848     +101     
- Misses        622      649      +27     
  Partials      256      256              

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

@Viicos Viicos force-pushed the feature/3688-registration-variables branch 2 times, most recently from b6d9596 to 01af7c6 Compare March 13, 2024 12:50
Viicos added 6 commits March 13, 2024 15:02
…gins

Update tests that where affected
For the objects API, the `variables_registry` will be defined in
the upcoming commits
Will be used by objects API registration variables
Define a new step, `save_registration_data`, that will
populate the `ObjectsAPIRegistrationData`.
@Viicos Viicos force-pushed the feature/3688-registration-variables branch from 5481e98 to 44bca03 Compare March 13, 2024 14:12
@Viicos Viicos marked this pull request as ready for review March 13, 2024 14:51
@Viicos Viicos requested a review from sergei-maertens March 13, 2024 14:51
Comment on lines +225 to +233
attachment_urls = ArrayField(
models.URLField(
_("attachment url"), help_text=_("The attachment URL on the Documents API.")
),
verbose_name=_("attachment urls"),
help_text=_("The list of attachment URLs on the Documents API."),
blank=True,
default=list,
)
Copy link
Member

Choose a reason for hiding this comment

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

I think this is too naive, you lose the information of which file/component the Document URL belongs too.

I'd still prefer to have the variable of a file upload explicitly mapped into the schema, e.g. something like:

type: object
properties:
  proofOfEmployment:
    type: string
    format: uri
  bankStatements:
    type: array
    items:
        type: string
        format: uri

so that you can make the (single) file upload "proof of employment" to /proofOfEmployment and the multi-file upload "bank statements" to /bankStatements

Possibly we do this in a separate PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Considering this requires thinking and probably some work, it will probably be part of the next PR

src/openforms/variables/api/registration_serializer.py Outdated Show resolved Hide resolved
src/openforms/variables/service.py Outdated Show resolved Hide resolved
src/openforms/variables/service.py Outdated Show resolved Hide resolved
Copy link
Member

@sergei-maertens sergei-maertens left a comment

Choose a reason for hiding this comment

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

Just need to fix CI :)

@Viicos Viicos force-pushed the feature/3688-registration-variables branch from f1bf228 to 86401a6 Compare March 14, 2024 14:24
@Viicos Viicos merged commit b1f5367 into master Mar 14, 2024
25 of 27 checks passed
@Viicos Viicos deleted the feature/3688-registration-variables branch March 14, 2024 14:36
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.

2 participants