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.

Sign up for GitHub

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

Jump to bottom

Add check max length Currency Code #1726

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

Add check max length Currency Code #1726

wants to merge 2 commits into from

Conversation

jaapmarcus
Copy link
Member

  • Change database password html element from password to text

@jaapmarcus
Add check for Currency Code
bd5ef69 
Max length 3 + A-Z  only
@jaapmarcus
Change "Database Password" from password to text
075c414 
Safari hates the fact that Database Password and "Admin" password are not the same and keeps it changing back every time ..
Anuril
Anuril requested changes Oct 17, 2023
View reviewed changes
Copy link
Contributor

@Anuril Anuril left a comment

Choose a reason for hiding this comment

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

The validation should happen in javascript imo.

src/install/install.php
*
* @return boolean
*/
private function validateCurrency(): bool
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, this should happen in javascript, not in the installation code, as that would lead to the user not being able to fix it before installing FOSSBilling. Also, validateCurrency isn't called anywhere in the installation code.

src/install/install.php
throw new Exception('Maximum currency title is 50 characters');
}
if (strlen($this->session->get('currency_format')) > 30) {
throw new Exception('Maximum currency title is 30 characters');
Copy link
Contributor

Choose a reason for hiding this comment

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

This exception text is the same as above and should be fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Anuril Anuril Anuril requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
installer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jaapmarcus @Anuril