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

Form schema update #1937

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

Form schema update #1937

wants to merge 10 commits into from

Conversation

Allcharles
Copy link
Contributor

Form schema update

Made changes to the form schema to allow for custom behaviours specific to our app

Changes

  • Added adminOnly property for fields which can only be enabled by admins
  • Added ownerOnly property for fields which can only be enabled by owners
  • Added renderFieldOnly property to filter out fields which are not to be used by the form

Problems

  • I believe allowOriginalDownload is an owner only property on the project model? This should be updated to use the new logic
  • May be worth adding createOnly and updateOnly properties in the future for more complex models

Final Checklist

  • Assign reviewers if you have permission
  • Assign labels if you have permission
  • Link issues related to PR
  • Ensure project linter is not producing any warnings (npm run lint)
  • Ensure build is passing on all browsers (npm run test:all)
  • Ensure CI build is passing
  • Ensure docker container is passing (docs)

@Allcharles Allcharles added enhancement New feature or request architecture Architectural changes to the software labels Jun 13, 2022
@Allcharles Allcharles requested a review from atruskie June 13, 2022 08:56
@Allcharles Allcharles self-assigned this Jun 13, 2022
Comment on lines 19 to 21
adminOnly?: boolean;
ownerOnly?: boolean;
renderFieldOnly?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

document these

Comment on lines 19 to 21
adminOnly?: boolean;
ownerOnly?: boolean;
renderFieldOnly?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

why not re-use the json-schema readOnly definition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

readOnly implies that the field should be visible, but not editable. A better fit would be hide, however that could prevent us from using more advanced forms in the future. Creating our own definition provides a more clear message of the intent of the field, and does not break any future compatibility with ngx-formly

Copy link
Member

Choose a reason for hiding this comment

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

The point of these schema definitions is that they're read from our swagger/openapi document. There is not an equivalent field there - in fact we use readOnly for this purpose.

You haven't explained the difference between renderFieldOnly and readOnly either

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renderFieldOnly completely removes a field from the form, and is only used by the render field component. It should not be rendered in the form, as it can cause issues in the case of associated models. readOnly would imply that the form contains the field, but the field was disabled.

As for reading these values from swagger/opernapi, as far as I was aware, we had dropped that constraint as it had little progress. However, given how the DetailViewComponent and RenderFieldComponent work, I suspect they will not be fully compatible regardless, and a more major change is required. Especially if the addition of a renderFieldOnly property is causing these problems

Copy link
Member

Choose a reason for hiding this comment

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

It should not be rendered in the form, as it can cause issues in the case of associated models.

What issues? Seems like a problem for the models.

would imply that the form contains the field, but the field was disabled.

Do you have a scenario where a field that is currently readOnly in our API is in actuality only temporarily disabled?

As for reading these values from swagger/opernapi, as far as I was aware, we had dropped that constraint as it had little progress

I am not aware, albeit it is not yet sufficient to serve that purpose for the whole API.
What I did say is that if we abandoned the idea we'd remove ngx-formly altogether.

Especially if the addition of a renderFieldOnly property is causing these problems

It's not causing problems, I just don't understand the need.

@atruskie
Copy link
Member

I believe allowOriginalDownload is an owner only property on the project model? This should be updated to use the new logic

Redundant. The whole item is only writeable by an owner.

May be worth adding createOnly and updateOnly properties in the future for more complex models

I've thought about this too, waiting on an official change from open API spec for this: OAI/OpenAPI-Specification#1497

@github-actions
Copy link
Contributor

Unit Test Results

         6 files           6 suites   10m 2s ⏱️
16 746 tests 16 086 ✔️ 660 💤 0
16 800 runs  16 140 ✔️ 660 💤 0

Results for commit 28f70e6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture Architectural changes to the software enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants