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

fix(bodies): replace SHOULD with MUST for mandatory requirements #414

Merged
merged 3 commits into from
Dec 28, 2021

Conversation

dilyanpalauzov
Copy link
Contributor

Remove default: '' for values, which must not be empty.

Emails must not be empty. CLE, MHW, ZRH have no emails.

Remove default: '' for values, which must not be empty.

Emails must not be empty.  CLE, MHW, ZRH have no emails.
@WikiRik
Copy link
Member

WikiRik commented Dec 28, 2021

Thanks for the PR! I have reconfigured CircleCI so it should do checks from forks as well, but I'm not sure when it kicks in unfortunately.

Copy link
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

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

Changes look good and are a good start for further refactoring.

Looking at this again I don't know why task_description needs a default value and why phone and address must be set as it's allowed to be null (which is also the behaviour we want). So I'll take a look at that soon as it's easy refactoring and a good moment to check the tests. Will then also replace SHOULD with MUST in the other models. Other modules of MyAEGEE will also get those refactoring updates later

@WikiRik WikiRik changed the title models/Body: replace SHOULD with MUST for mandatory requirements fix(bodies): replace SHOULD with MUST for mandatory requirements Dec 28, 2021
@codecov
Copy link

codecov bot commented Dec 28, 2021

Codecov Report

Merging #414 (d159ead) into master (6e9d7ff) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #414   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           42        42           
  Lines         1435      1435           
  Branches       231       231           
=========================================
  Hits          1435      1435           
Impacted Files Coverage Δ
models/AccessToken.js 100.00% <ø> (ø)
models/Body.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e9d7ff...d159ead. Read the comment docs.

Copy link
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

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

@WikiRik WikiRik merged commit f0a1211 into AEGEE:master Dec 28, 2021
serge1peshcoff pushed a commit that referenced this pull request Dec 28, 2021
## [1.36.10](1.36.9...1.36.10) (2021-12-28)

### Bug Fixes

* **bodies:** replace SHOULD with MUST for mandatory requirements ([#414](#414)) ([f0a1211](f0a1211))
@serge1peshcoff
Copy link
Member

🎉 This PR is included in version 1.36.10 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@dilyanpalauzov dilyanpalauzov deleted the should_must branch December 28, 2021 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants