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

Add frontend businessrules for Address isPrimary #4443

Merged
merged 6 commits into from
Feb 2, 2024
Merged

Conversation

melton-jason
Copy link
Contributor

@melton-jason melton-jason commented Jan 21, 2024

Fixes #4440
Addresses part of #672

#672 can additionally be closed if this Pull Request resolves that as well.
However, there still may be cases where Agent's can have multiple primary addresses: this change only handles the case where the isPrimary field is manually altered on one of the Agent's addresses, or a new Address is added.
The case of an Agent having multiple primary addresses before these changes is not one that I think should be handled or resolved automatically by way of a migration or business rule.

Because of this, I would reccomend closing #672, and instead open a new Issue which implements @maxpatiiuk's suggestion in #672 (comment)

@specify/ux-testing
There is a similar field on Address titled isCurrent.
Should the field also have a businessrule with the same behavior as isPrimary?

Checklist

  • Self-review the PR after opening it to make sure the changes look good
    and self-explanatory (or properly documented)
  • Add automated tests
  • Add relevant issue to release milestone

Testing instructions

On the Agent form, ensure the Address isPrimary field behaves the same as Determination isCurrent does.
More specifically:

  • Creating a new Address should uncheck isPrimary for all other Addresses and set the new Address isPrimary to true.
  • On making any Address primary on the Agent, isPrimary should be unchecked for all other addresses on the Agent.

@melton-jason melton-jason requested review from a team January 21, 2024 20:49
@CarolineDenis CarolineDenis added this to the 7.9.4 milestone Jan 22, 2024
@maxpatiiuk
Copy link
Member

There is a similar field on Address titled isCurrent.
Should the field also have a businessrule with the same behavior as isPrimary?

I might be missing it, but I don't see back-end having business rules for either of these fields in Address table

Though, in

on the front-end we seem to be force-hiding the isPrimary field (since 4861b7d)

(the form has both isCurrent and isPrimary though)


Determiner has isPrimary field too

Copy link
Contributor

@pashiav pashiav left a comment

Choose a reason for hiding this comment

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

Looks good and is working as expected!

Copy link
Member

@maxpatiiuk maxpatiiuk left a comment

Choose a reason for hiding this comment

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

👏👏👏
you are setting the next level for Specify with all those tests you add all the time

@melton-jason
Copy link
Contributor Author

melton-jason commented Feb 2, 2024

@specify/ux-testing
There were some changes made in a184d4c and this branch has been updated with the newest changes in production. Can this be tested again before it is merged?

@melton-jason melton-jason requested a review from a team February 2, 2024 16:54
Copy link
Collaborator

@emenslin emenslin left a comment

Choose a reason for hiding this comment

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

Looks good, is primary can only be selected for one address.

@melton-jason melton-jason merged commit 1e6d680 into production Feb 2, 2024
9 checks passed
@melton-jason melton-jason deleted the issue-4440 branch February 2, 2024 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Primary Address doesn't match functionality of Current determination
5 participants