Skip to content
This repository has been archived by the owner on Mar 24, 2023. It is now read-only.

fix boefje settings forms for integer fields #91

Merged
merged 5 commits into from
Dec 28, 2022

Conversation

noamblitz
Copy link
Contributor

@noamblitz noamblitz commented Dec 22, 2022

Changes

The boefje settings form did not work for integer fields because of the max_length field.

Proof

image

Good to know

  • Does this PR introduce or depend on API-incompatible changes? If yes: what do other users/developers need to do or confirm before merging?
    -No
  • Does this PR depend on a specific version of a library?
    -No
  • Does this PR require config, setup, or .env changes?
    -No

Checklist for author(s):

  • This PR comes from a feature or hotfix branch, in line with our git branching strategy;
  • I have squashed all commits before merging this PR;
  • This PR is "bite-sized" and only focuses on a single issue, problem, or feature;
  • If a non-trivial PR: This PR is properly linked to an issue on the project board;
  • If a non-trivial PR: I have added screenshots or some other proof that my code does what it is supposed to do;
  • I am not reinventing the wheel: there is no high-quality library that already has this feature;
  • I have changed the example .env files if I added, removed, or changed any config options, and I have informed others that they need to modify their .env files if required;
  • I have performed a self-review of my own code;
  • I have commented my code, particularly in hard-to-understand areas;
  • [ x I have made corresponding changes to the documentation, if necessary;
  • I have written unit, integration, and end-to-end tests for the change that I made;
## Checklist for functional reviewer(s):
- [ ] If a non-trivial PR: This PR is properly linked to an issue on the project board;
- [ ] I have checked out this branch, and successfully ran `make kat`;
- [ ] I have ran `make test-rf` and all end-to-end Robot Framework tests pass;
- [ ] I confirmed that the PR's advertised `feature` or `hotfix` works as intended;
- [ ] I confirmed that there are no unintended functional regressions in this branch;

### What works:
* _bullet point + screenshot (if useful) per tested functionality_

### What doesn't work:
* _bullet point + screenshot (if useful) per tested functionality_

### Bug or feature?:
* _bullet point + screenshot (if useful) if it is unclear whether something is a bug or an intended feature._
## Checklist for code reviewer(s):
- [ ] The code passes the CI tests and linters;
- [ ] The code does not bypass authentication or security mechanisms;
- [ ] The code does not introduce any dependency on a library that has not been properly vetted;
- [ ] The code does not violate Model-View-Template and our other architectural principles;
- [ ] The code contains docstrings, comments, and documentation where needed;
- [ ] The code prioritizes readability over performance where appropriate;
- [ ] The code conforms to our agreed coding standards.

@noamblitz noamblitz requested a review from a team as a code owner December 22, 2022 12:21
@underdarknl
Copy link
Contributor

can we make sure to implement the 2 json schema keywords 'minimum' and 'maximum' for number and integer fields?
Also, for integer fields, we should set the html "step" argument to 1.
https://json-schema.org/understanding-json-schema/reference/numeric.html#number

@noamblitz
Copy link
Contributor Author

Sounds like a good idea! But i would prefer to create a ticket for that as this is purely meant as a bugfix pr

ammar92
ammar92 previously approved these changes Dec 26, 2022
@underdarknl
Copy link
Contributor

Sounds like a good idea! But i would prefer to create a ticket for that as this is purely meant as a bugfix pr

Agreed. I've asked @HeleenSG to design these pages for more complex forms anyway, which would allow us to use more complete JsonSchema's and render those.

@underdarknl underdarknl merged commit 81f5150 into main Dec 28, 2022
@underdarknl underdarknl deleted the fix/boefje-settings-forms branch December 28, 2022 12:27
jpbruinsslot added a commit that referenced this pull request Dec 29, 2022
* main:
  adding cached network reference support to csv importer. (#95)
  fix boefje settings forms for integer fields (#91)
  Fix ignored flake8 errors and change tests to pytest (#87)
  add missing dmarc, dkim and spf findings (#35)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants