PolicyEngine / policyengine-app

PolicyEngine's free web app for computing the impact of public policy.
GNU Affero General Public License v3.0
35 stars 89 forks source link

Add prompt for duplicate policy names #972

Open nikhilwoodruff opened 5 months ago

nikhilwoodruff commented 5 months ago

When a user names a policy with a name that already exists, we should prompt them with two options:

anth-volk commented 5 months ago

I hate to ask this, but wouldn’t the second be confusing for the user whose policy just lost its name?

On Dec 16, 2023, at 10:37, Nikhil Woodruff @.***> wrote:

 When a user names a policy with a name that already exists, we should prompt them with two options:

Choose a different name Un-name the old one (revert to Policy #X) and keep the current name — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you are subscribed to this thread.

MaxGhenis commented 5 months ago

Longer term we may want to restrict policy naming to signed-in users and requiring unique names at the user-policy name level, at which point this becomes less of an issue.

nikhilwoodruff commented 5 months ago

I hate to ask this, but wouldn’t the second be confusing for the user whose policy just lost its name?

@anth-volk yes but I think it's more confusing that there are duplicates.

anth-volk commented 5 months ago

@nikhilwoodruff Honestly, I think it's more confusing to suddenly have a lost policy name.

I would propose dropping the second part and only requiring users to utilize unique names for the time being; alternatively, as a quick workaround, we could always display the policy number in front of the name? That would at least somewhat disambiguate.

Then, in a future redesign, I'd propose allowing users to explicitly make their policies public, then similarly either enforcing unique names if made public or displaying the policy number in front, or perhaps something different.

nikhilwoodruff commented 5 months ago

OK yep sure, that works for now

dotslashbit commented 4 months ago

@anth-volk I want to work on this, whats the final decision on the approach?

anth-volk commented 4 months ago

@dotslashbit At the moment, we'd like to enforce that, when a user proposes a name for a policy, that this name must be unique. Ideally, the component should verify an input name against a list of all existing names, and if there is a match, it should display some sort of error message without dropping what the user has already input (in case they accidentally clicked off the component or something like that) and without having renamed the policy from its default or its previously saved name.

It would be great if you could also find a way to do this without having to write a new API endpoint to query all existing policy names and without having some sort of call with a massive load overhead, e.g., querying all policies in the database or something like that. If it's unavoidable, though, we can open a back-end issue to accomplish this.

dotslashbit commented 4 months ago

@anth-volk ok, working on it

dotslashbit commented 4 months ago

@anth-volk Can you please share a video on how currently the user can create a reform. I can;t figure out how to do that

anth-volk commented 4 months ago

@dotslashbit Sure, I can do so, but it'll be either later tonight or tomorrow

dotslashbit commented 4 months ago

That's totally fine with me

dotslashbit commented 4 months ago

@anth-volk There are possibly two ways to check all the existing policy names:

  1. By querying all the policies in the database
  2. By keeping policy ID names unique to respective user ( let me explain) Let's say there are two users A and B, both can have their own policy names as "my new policy". Even though the policy name isn't unique but its unique for that particular user. Let me know what do you think about it and if you need any clarification then I can explain
anth-volk commented 4 months ago

@dotslashbit Unfortunately no. 2 doesn't do what we're looking for. It'd be great after a future rewrite of the calculator component, which we're hoping to include a login experience within, but at the moment, every user has access to all policies that have ever been created.

dotslashbit commented 4 months ago

@anth-volk So do you have any idea on how to approach this, or should I implement the first approach or should I stop working on this and after inlcuding the sign-in feature, I should work in this?

anth-volk commented 4 months ago

@dotslashbit This could use some more technical specification, couldn't it? I do think we should put something stopgap in place until we build out a new calculator UI, as it's unclear when we'll have the capability to do it. If you can give me until a bit later tonight, I can work something more definitive up.

dotslashbit commented 4 months ago

ok, will wait for your input on this, thanks

anth-volk commented 4 months ago

@dotslashbit Okay, I should've thought of this before, but we already use the API's search endpoint within the PolicySearch front-end component. I think that all we'd have to do in this case is either augment, expand upon, or duplicate the logic there so taht as the user is creating a name, we're confirming that there isn't that specific name in the database.

Additionally, I think we should pair this with a button that I mentioned in #1190 that the user has to press to rename a policy so that this search doesn't run unless the user's renaming. Ideally, the search shouldn't run until the user presses to submit; at that point, we should emit the request, and if it's found that a policy already exists with the same name, we should display a validation message (ideally that doesn't move all the other components downward - feel free to put it where you think it should go on your first edits, and we can discuss placement afterward?) that tells the user that their name is already taken.

dotslashbit commented 4 months ago

@anth-volk This makes sense, okay will start to look into this from now, thanks. So, should I create a draft pr for now for this issue and #1190. Thinking of keeping these two in the same PR

anth-volk commented 4 months ago

@dotslashbit Exactly, please do

cats256 commented 2 months ago

Shouldn't this be closed?

anth-volk commented 2 months ago

We probably need confirmation on this, actually. The original idea had been to force users to always adopt a unique name, even after we got rid of the old duplicate policies. That said, I'm wondering if we should do this. Let me confirm at some point this week.

anth-volk commented 2 months ago

Confirmed, we will be working on this.