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

Create format validation for phone nos. #1442

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

Conversation

DonHendriksen
Copy link

@DonHendriksen DonHendriksen commented Jul 1, 2024

Summary

Provides methods to validate that entered data is in the correct format, such as phone numbers.

Work Item(s)

Fixes #1493

@DonHendriksen DonHendriksen requested a review from a team as a code owner July 1, 2024 08:55
@github-actions github-actions bot added AL: System Application From Fork Pull request is coming from a fork labels Jul 1, 2024
@pri-kise
Copy link
Contributor

pri-kise commented Jul 3, 2024

Some comments on this small PR.

  1. I kind of like the idea to have a format validation codeunit in the System Application. Since there can be other data type that we want to check in the future I'd suggest to rename the codeunit to a more generic name. (e.g. Codeunit Format Validation or Codeunit Check Format)
  2. Although this is codeunit does not contain much code you should follow the facade pattern.
    Every procedure in the public codeunit should contain a documentation header.
  3. Can you maybe add a 2nd public procedure that accepts an PhoneNo Text as Input and simple returns a boolean when the format is valid
  4. You cannot depend on BaseApp Objects on tests for the system application
    If you want to test this logic in the System Application then you maybe need to create a new Test Table that contains a Phone No Text field for this purpose.

@JesperSchulz JesperSchulz added the Integration GitHub request for Integration area label Jul 3, 2024
@DonHendriksen
Copy link
Author

@pri-kise, I processed the first 3 remarks. Implemented the facade pattern. Please have a first look.

Some questions:

  1. I need a work item. I cannot link it to the existing one [releases/24.0] Update BC Artifact version #767. Do we need to create a new issue for it?
  2. I created a new app for Format Validation. Can you provide me, for production and test app, the app id and the object number range? If possible check dependencies and brief/description. Any suggestions are welcome.
  3. What do you think of the public procedures in the facade codeunit?
  4. For testing purposes I can create a new app in the Test Library. A simple FormatValidation table which currently only contains a phone no. field. And this table can be used to test the facade codeunit. Is this approach okay?

@pri-kise
Copy link
Contributor

pri-kise commented Jul 4, 2024

Regaring your questions:

I need a work item. I cannot link it to the existing one #767. Do we need to create a new issue for it?

It would be the easiest if you you create an issue on this repository. @JesperSchulz can then approve it.

I created a new app for Format Validation. Can you provide me, for production and test app, the app id and the object number range? If possible check dependencies and brief/description. Any suggestions are welcome.

For the appId. -> Simple take a new Guid
Number Ranges are always a littel bit tricky. Searching fro free Ids isn't that easy.. Maybe @JesperSchulz can help out on this.

What do you think of the public procedures in the facade codeunit?

lgtm

For testing purposes I can create a new app in the Test Library. A simple FormatValidation table which currently only contains a phone no. field. And this table can be used to test the facade codeunit. Is this approach okay?

I think this would be the correct approach.
New Test Library with a simple table.
I think this is a valid approach. Similiar was done for the translation module. https://github.com/microsoft/BCApps/blob/main/src/System%20Application/Test%20Library/Translation/src/TranslationTestTable.Table.al
I would suggest to use an Integer as primary key (similiar to the translation module)

@pri-kise
Copy link
Contributor

pri-kise commented Jul 5, 2024

In general this PR is now in a good shape for review by someone of Microsoft. 👍

@DonHendriksen
Copy link
Author

@pri-kise @StefanSosic What are the next steps? I want to keep this PR rolling.

I still need object nos before I can build and publish the changes. I currently cannot test the changes in this PR. And if this one is approved I still have the PR in the BusinessCentralApps repo pending, to call these new validations from Contact Alt. Address.

Copy link
Contributor

@JesperSchulz JesperSchulz left a comment

Choose a reason for hiding this comment

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

Let's address the ID question first. Generally the design guidelines are now followed and this is a proper module, but I'm not sure I'm a fan of the two public procedures. I will set aside time on Monday to take a closer look.


namespace System.Test.Utilities;

codeunit 139194 "Format Validation Tests"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use ID 130449.

Copy link
Author

Choose a reason for hiding this comment

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

This object number is also assigned to the test table.

Caption = 'Format Validation Test Table';
DataClassification = SystemMetadata;

fields
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it really necessary to introduce this table just to test the validation of a phone no? I'd argue it's not.

Copy link
Author

Choose a reason for hiding this comment

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

It is not necessary. In the unit test codeunit it is also possible to assign a value without using a table. Please discuss it internally because this was the suggestion from your colleagues.

@JesperSchulz
Copy link
Contributor

I've created an issue for you and have linked everything up alright. After code review on Monday we'll settle on a final design and then we'll get this one in before the week is over. I hope that works for you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AL: System Application From Fork Pull request is coming from a fork Integration GitHub request for Integration area
Projects
None yet
4 participants