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

FS-4895: When creating a fund or a round check, whether given round shortnames are available #149

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

Conversation

nuwan-samarasinghe
Copy link
Contributor

Ticket: https://mhclgdigital.atlassian.net/browse/FS-4895

Change description

In this change we are focussing on fund and round short name reuse issue so following scenarios are acceptable

  1. If there is a fund and that given short name is not in our records that short name is acceptable
  2. If there is an update for the given fund short name is acceptable because it belongs to it self
  3. IF there is a fund and when round creation if the short name is not available then acceptable
  4. IF there is a different fund and we are planing to use same short name it is acceptable
  5. If we do update the round then it will be again acceptable since short name belongs to it self

Following are not acceptable

  1. If there is a fund for a given short name then we are shoving an error for validation purpose
  2. If there is a fund and inside that fund if we have same short name round we give an error
  • Unit tests and other appropriate tests added or updated
  • README and other documentation has been updated / added (if needed)
  • Commit messages are meaningful and follow good commit message guidelines (e.g. "FS-XXXX: Add margin to nav items preventing overlapping of logo")

How to test

Testing Fund create

  1. Use FAB
  2. Try to create a fund that does not exists then it should show the success
  3. Try to create a existing short named fund it gives error
  4. Try to rename a short name of a fund to a already existing one then it should give an error other wise should be success

Testing Round Create

  1. Use FAB
  2. Try to rename a short name of a round to a already existing one then it should give an error other wise should be success

Screenshots of UI changes (if applicable)

image

image

@wjrm500 wjrm500 force-pushed the FS-4895-check-for-short-name-existance branch from a953359 to 90740d6 Compare December 13, 2024 15:57
Copy link
Contributor

@wjrm500 wjrm500 left a comment

Choose a reason for hiding this comment

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

Hey @nuwan-samarasinghe, I think a better implementation would be to create validation functions and attach them directly to the FlaskForms e.g.,

In apps/funding-service-design-fund-application-builder/app/blueprints/fund_builder/forms/fund.py:

...
def unique_fund_short_name(form, field):
    fund = get_fund_by_short_name(field.data)
    if fund and str(fund.fund_id) != form.fund_id.data:
        raise ValidationError("Given fund short name already exists.")
...
class FundForm(FlaskForm):
    ...
    short_name = StringField(
        "Short name",
        validators=[
            DataRequired(),
            Length(max=10),
            no_spaces_between_letters,
            unique_fund_short_name,
        ],
    )
    ...

This way we don't need to modify the route function or the error formatter at all, and we achieve the same result.

We can then do the same for Round. Let me know what you reckon!

Copy link

sonarcloud bot commented Dec 16, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
4 Security Hotspots
35.8% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

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.

3 participants