Add form for reassigning users to places #257 #258

freddieptf · 2025-02-25T00:42:40Z

Part of #185
Fixes #260

This adds the form for re-assigning a user to a place.

Benmuiruri

Hi @freddieptf

Thanks for this. I interacted with it but it was unsuccessful.

I was getting invalid link due to the regex not matching correctly.
After tinkering with the regex I was able to see it work but I have a question. In the Admin app I can see the place has been added to the new contact westi and when I login I can see the places

Reassign place

However, I still see the place when I login as the user test. Is that the expected behavior after re-assigning ?

Benmuiruri · 2025-02-25T08:32:39Z

src/routes/reassign.ts

+    const body =  req.body as { [key:string]:string };
+    const contactType = Config.getContactType(body.place_type);
+
+    const uuidMatch = body.contact.match('/contacts/(?<uuid>[a-z0-9]{32})');


This regex is not correct.

I don't think you should use the string inside the match because that will literally search for the string '/contacts/(?<uuid>[a-z0-9]{32})' instead of using it as a pattern.

The regex should also match hyphens that are in the contact uuid.

The regex is cause of a weird thing i found, place UUIDs had hyphens but people did not. I thought this was consistent behavior but I just checked on a live instance and it seems this is not always the case. I'll update this

I don't think you should use the string inside the match because that will literally search for the string '/contacts/(?[a-z0-9]{32})' instead of using it as a pattern.

The regex worked for me when i had UUIDs without hyphens

freddieptf · 2025-02-25T09:13:59Z

However, I still see the place when I login as the user test. Is that the expected behavior after re-assigning ?

@Benmuiruri good catch, I'll update this to remove the place from the other user and if the user only had one place assigned then i can just delete them

Benmuiruri

I was getting this error.

Error

But I now see you changed the implementation. Let me continue with the review

Benmuiruri

Nice work @freddieptf the reassignment works.

I have not checked everything but here is one thing I found. When you reassign a place from person A to person B. We change the contact person of the place and replace it with person B. If you want to reassign the place back to person A, how would one go about that? We now cannot find the person. Check attached video

Contact disappears

Screen.Recording.2025-02-28.at.18.20.53.mov

freddieptf · 2025-02-28T16:09:29Z

The behavior you observed was due to the fact that the contact was only supervising one place. If a contact is only supervising one place and the place gets assigned to another person, then their user account gets disabled and their contact gets deleted.

If you want to reassign the place back to person A, how would one go about that?

They'd have to get their contact and user created manually on the instance. This process can be improved in #257

Benmuiruri

The happy path works to reassign a CHU but I have left some comments and questions that could make the implementation more robust (in terms of error handling) and DRY. I highlighted some places you should consider error handling but review the various API calls and data processing to ensure things fail as expected.

Suggested improvements

When you submit an incorrect uuid the application fails silently by logging the error message but when you click done, the page breaks with Site cannot be reached
when you submit a correct contact link but that person is a CHP it shows failure 400, that error message should better communicate to the user why it failed (Selected contact is not a CHA)
when you submit a correct contact link of a person who is not a CHA it fails silently in the terminal but the page breaks and you get Site cannot be reached

Silent failure

When reassignment is successful, we could improve the feedback to the user with something like Successfully reassigned place xxx to user yyy in green. The success is good but it is immediately clear that the process succeeded.

src/lib/cht-api.ts

Benmuiruri · 2025-03-03T06:41:32Z

src/lib/cht-api.ts

@@ -184,6 +193,16 @@ export class ChtApi {
    }));
  }

+  async getUser(contactId: string): Promise<UserInfo> {
+    const url = `api/v2/users?contact_id=${contactId}`;
+    console.log('axios.get', url);


delete console.log

These are actually needed when debugging both in dev and prod, that's why every request in cht-api has a log statement

src/routes/reassign.ts

Benmuiruri · 2025-03-03T06:58:03Z

src/routes/reassign.ts

+
+  fastify.get('/reassign', async (req, resp) => {
+    const { place_type } = req.query as any;
+    const contactType = Config.getContactType(place_type);


Do we need error handling here if getContactType fails ?

Not strictly, the user would have to manually edit the query param in the URL for this to fail

kennsippell · 2025-03-03T09:41:56Z

src/liquid/reassign/form.liquid

+        >
+          https://{{ session.authInfo.domain -}}
+        </a>
+        then copy and paste their link below


It is common throughout the UMT to need to "look up information in eCHIS". In all cases, the user types the name of whatever contact they are looking for.

This is quite a paradigm shift here! We are breaking the pattern and expecting completely different information. It requires the use of a browser instead of using the app. I think it will increase training burdens.

Why can't the user provide the standard inputs to specific a CHU? CHUs have only one primary contact and so the CHA can be specified easily through existing means. I think it makes sense to allow users to provide a UUID any time they are prompted to search for a place. Like you can type "Matunga" or you can copy/paste the place's UUID.

I'm concerned about supporting only UUIDs in this single part of the UI here...
Can you share your thinking on why your approach is so different?

In all cases, the user types the name of whatever contact they are looking for.

I don't feel like we can guarantee a performant search as is, that's why i opted for copying the URL from the CHT. If we can provide the same "general search" experience as the CHT then i think that might be better than copying the URL/UUID especially if we can get the contact hierarchy in the results as well.

kennsippell · 2025-03-03T09:46:55Z

src/liquid/app/nav.html

@@ -35,6 +35,9 @@
        </a>
        <div class="navbar-dropdown">
          {% for placeType in contactTypes %}
+            {% if placeType.can_assign_multiple %}


I somehow don't see the option to add anymore?

It was never there, you click "Add x" as before and if the instance version is >= 4.9.0 and can_assign_multiple is set then it loads the new form

kennsippell · 2025-03-03T09:53:19Z

src/liquid/reassign/form.liquid

@@ -0,0 +1,132 @@
+<div


i think this experience would be more intuitive if the elements on the form were reordered. what do you think of having "stuff there is only one of" first (CHA information) and the "n element dynamic content with table" at the end?

My thinking here is since CHU's are the focus of re-assignment, they should be first and have more emphasis. I'm trying to make that clear via the UI. Placing the CHA first could make it seem like the CHA will be reassigned to the areas the user is adding and removed from the areas they already supervise

kennsippell · 2025-03-03T09:53:42Z

src/liquid/reassign/form.liquid

+    <input name="place_type" value="{{contactType.name}}" hidden>
+
+    <div class="p-2 mt-2" style="border-left: thick solid green;">
+      <p>Search and add a {{ contactType.friendly }} below</p>


Instead of these three
tags, I suspect it would be clearer if they were just titles.

"Specify the CHA receiving reassignments"

"Add the CHUs to reassign"

"Staged Reassignments"

kennsippell · 2025-03-03T10:50:29Z

src/liquid/reassign/form.liquid

+      </div>
+    </div>
+
+    {% if places and places.length > 0 %}


This is another paradigm shift to have the table, staging, and upload progress here on the form instead of on the homepage. I have two broad concerns.

First concern is that I'm unclear how you're going to implement CSV import for CHA reassignment and creation. Will there be another form with another table? Rows don't appear editable presently which is key to CSV import experience. Do warnings work? How do you retry if things fail (I tested and didn't see an error).

Would you provide a mockup of how you're expecting the top-level nav to work for this? I think CSV important is really important, so it'd be good to think some of these things through and get on the same page before proceeding too far.

My second concern is just around consistency of the experience. Currently staging and uploading is orchestrated on the home page. I'm not saying what is done here is undesirable, but it is inconsistent.

Would you provide a mockup of how you're expecting the top-level nav to work for this? I think CSV important is really important, so it'd be good to think some of these things through and get on the same page before proceeding too far.

All the items under Manage on the navbar should have a status page/list of their own. We can have tabs on the homepage or different pages for these

Will there be another form with another table? Rows don't appear editable presently which is key to CSV import experience. Do warnings work?

Yes there will be another form and table reusing the views in this one. I don't think we need warnings/validation as the places are already created.

How do you retry if things fail (I tested and didn't see an error).

Right now, unfortunately, restart from scratch. This will be improved

Benmuiruri self-requested a review February 25, 2025 06:08

freddieptf linked an issue Feb 25, 2025 that may be closed by this pull request

Add form for reassigning users to places #260

Open

Benmuiruri requested changes Feb 25, 2025

View reviewed changes

freddieptf requested a review from Benmuiruri February 26, 2025 23:22

Benmuiruri reviewed Feb 28, 2025

View reviewed changes

Benmuiruri requested changes Mar 3, 2025

View reviewed changes

kennsippell reviewed Mar 3, 2025

View reviewed changes

freddieptf force-pushed the 185_reassignment branch from 8b7f131 to d3756dc Compare March 5, 2025 15:55

freddieptf added 6 commits March 12, 2025 16:37

add form for reassigning

093c6c5

delete the reassigned place association

ac4f5bd

fix link

cf3eb8e

lets handle multiple

d1287af

lint

b0fdb16

errors

87b4f23

freddieptf force-pushed the 185_reassignment branch from d3756dc to 87b4f23 Compare March 12, 2025 20:07

limit reassignment to parent levels

a7b6397

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add form for reassigning users to places #257 #258

Add form for reassigning users to places #257 #258

freddieptf commented Feb 25, 2025 •

edited

Loading

Benmuiruri left a comment

Benmuiruri Feb 25, 2025

freddieptf Feb 25, 2025

freddieptf Feb 25, 2025

freddieptf commented Feb 25, 2025

Benmuiruri left a comment •

edited

Loading

Benmuiruri left a comment

freddieptf commented Feb 28, 2025

Benmuiruri left a comment •

edited

Loading

Benmuiruri Mar 3, 2025

freddieptf Mar 3, 2025

Benmuiruri Mar 3, 2025

freddieptf Mar 3, 2025

kennsippell Mar 3, 2025

freddieptf Mar 3, 2025

kennsippell Mar 3, 2025

freddieptf Mar 3, 2025

kennsippell Mar 3, 2025

freddieptf Mar 5, 2025

kennsippell Mar 3, 2025

kennsippell Mar 3, 2025

freddieptf Mar 3, 2025

Add form for reassigning users to places #257 #258

Are you sure you want to change the base?

Add form for reassigning users to places #257 #258

Conversation

freddieptf commented Feb 25, 2025 • edited Loading

Benmuiruri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freddieptf commented Feb 25, 2025

Benmuiruri left a comment • edited Loading

Choose a reason for hiding this comment

Benmuiruri left a comment

Choose a reason for hiding this comment

freddieptf commented Feb 28, 2025

Benmuiruri left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freddieptf commented Feb 25, 2025 •

edited

Loading

Benmuiruri left a comment •

edited

Loading

Benmuiruri left a comment •

edited

Loading