fix(#9601): prototype duplicate prevention #9609
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| <div | ||
| id="duplicate_info" | ||
| [ngStyle]="{ display: duplicates.length > 0 ? 'block' : 'none' }"> | ||
| <p class="results_header">{{ duplicates.length }} {{'potential duplicate item(s) found:' | translate }}</p> | ||
| <div class="divider"></div> | ||
| <div> | ||
| <label for="check" class="acknowledge_label"> | ||
| <input | ||
| id="check" | ||
| type="checkbox" | ||
| [checked]="acknowledged" | ||
| (change)="toggleAcknowledged()" | ||
| class="acknowledge_checkbox"/> | ||
| {{'Acknowledge duplicate siblings and proceed with submission' | translate }} | ||
| </label> | ||
| </div> | ||
| <div class="divider"></div> | ||
| <div *ngFor="let duplicate of duplicates; let i = index" > | ||
| <div class="card"> | ||
| <strong>{{ 'Item number:' | translate }}</strong> {{ i + 1 }} | ||
| <hr> | ||
| <p> | ||
| <strong>{{ 'Name:' | translate }}</strong> {{ duplicate.name }} | ||
| <br> | ||
| <strong>{{ 'Created on:' | translate }}</strong> {{ duplicate.reported_date | date: 'EEE MMM dd yyyy HH:mm:ss' }} | ||
| </p> | ||
| <button class="toggle-button" (click)="toggleSection(getPath('', duplicate._id))"> | ||
| {{ isExpanded(getPath('', duplicate._id)) ? '▼' : '▶' }} | ||
| {{ (isExpanded(getPath('', duplicate._id)) ? 'Show less details' : 'Show more details') | translate }} | ||
| </button> | ||
| <div *ngIf="isExpanded(getPath('', duplicate._id))" class="nested-section"> | ||
| <ng-container *ngTemplateOutlet="renderObject; context: { obj: duplicate, path: duplicate._id }"></ng-container> | ||
| </div> | ||
| <hr> | ||
| <button class="btn submit btn-primary" (click)="_navigateToDuplicate(duplicate._id)">{{ 'Take me there' | translate }}</button> | ||
| </div> | ||
| </div> | ||
| <ng-template #renderObject let-obj="obj" let-path="path"> | ||
| <div *ngFor="let key of obj | keyvalue"> | ||
| <div *ngIf="isObject(key.value); else primitiveValue"> | ||
| <button class="toggle-button" (click)="toggleSection(getPath(path, key.key))"> | ||
| {{ isExpanded(getPath(path, key.key)) ? '▼' : '▶' }} {{ key.key }} | ||
| </button> | ||
| <div *ngIf="isExpanded(getPath(path, key.key))" class="nested-section"> | ||
| <ng-container *ngTemplateOutlet="renderObject; context: { obj: key.value, path: getPath(path, key.key) }"></ng-container> | ||
| </div> | ||
| </div> | ||
| <ng-template #primitiveValue> | ||
| <strong>{{ key.key }}:</strong> {{ key.value }} | ||
| </ng-template> | ||
| </div> | ||
| </ng-template> | ||
| </div> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| import { Component, EventEmitter, Input, Output } from '@angular/core'; | ||
|
|
||
| @Component({ | ||
| selector: 'mm-duplicate-info', | ||
| templateUrl: './duplicate-info.component.html', | ||
| }) | ||
| export class DuplicateInfoComponent { | ||
| @Input() acknowledged: boolean = false; | ||
| @Output() acknowledgedChange = new EventEmitter<boolean>(); | ||
| @Output() navigateToDuplicate = new EventEmitter<string>(); | ||
| @Input() duplicates: { _id: string; name: string; reported_date: string | Date; [key: string]: string | Date }[] = []; | ||
|
|
||
| toggleAcknowledged() { | ||
| this.acknowledged = !this.acknowledged; | ||
| this.acknowledgedChange.emit(this.acknowledged); | ||
| } | ||
|
|
||
| _navigateToDuplicate(_id: string){ | ||
| this.navigateToDuplicate.emit(_id); | ||
| } | ||
|
|
||
| // Handles collapse / expand of duplicate doc details | ||
| expandedSections = new Map<string, boolean>(); | ||
|
|
||
| toggleSection(path: string): void { | ||
| this.expandedSections.set(path, !this.expandedSections.get(path)); | ||
| } | ||
|
|
||
| isExpanded(path: string): boolean { | ||
| return this.expandedSections.get(path) || false; | ||
| } | ||
|
|
||
| isObject(value: any): boolean { | ||
| return value && typeof value === 'object' && !Array.isArray(value); | ||
| } | ||
|
|
||
| getPath(parentPath: string, key: string): string { | ||
| return parentPath ? `${parentPath}.${key}` : key; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -5,7 +5,7 @@ import { isEqual as _isEqual } from 'lodash-es'; | |||||
| import { ActivatedRoute, Router } from '@angular/router'; | ||||||
|
|
||||||
| import { LineageModelGeneratorService } from '@mm-services/lineage-model-generator.service'; | ||||||
| import { FormService, DuplicatesFoundError, Duplicate } from '@mm-services/form.service'; | ||||||
| import { EnketoFormContext } from '@mm-services/enketo.service'; | ||||||
| import { ContactTypesService } from '@mm-services/contact-types.service'; | ||||||
| import { DbService } from '@mm-services/db.service'; | ||||||
|
|
@@ -55,6 +55,18 @@ export class ContactsEditComponent implements OnInit, OnDestroy, AfterViewInit { | |||||
| private trackSave; | ||||||
| private trackMetadata = { action: '', form: '' }; | ||||||
|
|
||||||
| private duplicateCheck; | ||||||
| acknowledged = false; | ||||||
| onAcknowledgeChange(value: boolean) { | ||||||
| this.acknowledged = value; | ||||||
| } | ||||||
|
|
||||||
| onNavigateToDuplicate(_id: string){ | ||||||
| this.router.navigate(['/contacts', _id, 'edit']); | ||||||
|
There was a problem hiding this comment. question: When i clicked the "Take me there" button, I had expected to go to the contact's profile page. Why do we take the user straight to the edit form? There was a problem hiding this comment. Initially we thought that the user might not have enough details around why the sibling was flagged as a duplicate, which can cause frustration. There was a problem hiding this comment. Yeah, that is a good point that this could be affected by the conclusions of the UX discussion.
Speaking purely from my opinion (with no real-world data to back me up), my expectation would be that any info important enough to be dupe-checking on would also be important enough to display via the contact-summary on a contact's profile page. I can see where you are coming from with the consistency of moving from entering data in a contact create form to now being in the contact edit form were we can review the data and make any necessary changes. On the other hand, though, I am sure we will get cases like this:
Ultimately, you and your team are closer to the end-users then I am and I will happily defer to your expertise with these kinds of things! I hope it is useful for me to raise questions and point out other possible behaviors we can discuss, but ultimately I am not any kind of UX expert nor do I have direct access to regularly observe/interact-with CHT users. There was a problem hiding this comment. You make a good point. Making decisions for the user without fully understanding their behavior and needs could backfire. Navigating to the profile screen, where they have the option to do x, y, or z, is better than forcing them into an edit. That said, IMO we should provide the user with enough information before navigating to the item, as they will lose the form data. This is especially important when considering multiple duplicates, as discussed in the forum thread. |
||||||
| } | ||||||
|
|
||||||
| duplicates: Duplicate[] = []; | ||||||
|
There was a problem hiding this comment. suggestion: WebStorm is complaining to me that the |
||||||
|
|
||||||
| ngOnInit() { | ||||||
| this.trackRender = this.performanceService.track(); | ||||||
| this.subscribeToStore(); | ||||||
|
|
@@ -153,6 +165,10 @@ export class ContactsEditComponent implements OnInit, OnDestroy, AfterViewInit { | |||||
| this.contentError = false; | ||||||
| this.errorTranslationKey = false; | ||||||
|
|
||||||
| // Reset when when navigated to duplicate | ||||||
|
There was a problem hiding this comment. nitpick: this comment might be misleading since this will get called anytime a new form is opened (not just when navigating to the duplicate contact). I think it is clear from the context of this function that we are simply resetting the stored form state for a new form.
Suggested change
There was a problem hiding this comment. Agreed. Clearing the values will also be unnecessary if we navigate to the profile page instead of switching out the edit screen content. |
||||||
| this.duplicates = []; | ||||||
| this.acknowledged = false; | ||||||
|
|
||||||
| try { | ||||||
| const contact = await this.getContact(); | ||||||
| const contactTypeId = this.contactTypesService.getTypeId(contact) || this.routeSnapshot.params?.type; | ||||||
|
|
@@ -272,6 +288,7 @@ export class ContactsEditComponent implements OnInit, OnDestroy, AfterViewInit { | |||||
| private async renderForm(formId: string, titleKey: string) { | ||||||
| const formDoc = await this.dbService.get().get(formId); | ||||||
| this.xmlVersion = formDoc.xmlVersion; | ||||||
| this.duplicateCheck = formDoc.context?.duplicate_check; | ||||||
|
There was a problem hiding this comment. issue: conceptually, I don't think we want to put
Suggested change
There was a problem hiding this comment. We will need a small update to cht-conf to support uploading the |
||||||
|
|
||||||
| this.globalActions.setEnketoEditedStatus(false); | ||||||
|
|
||||||
|
|
@@ -326,7 +343,9 @@ export class ContactsEditComponent implements OnInit, OnDestroy, AfterViewInit { | |||||
| $('form.or').trigger('beforesave'); | ||||||
|
|
||||||
| return this.formService | ||||||
| .saveContact({ | ||||||
| form, docId, type: this.enketoContact.type, xmlVersion: this.xmlVersion | ||||||
| }, this.duplicateCheck, this.acknowledged) | ||||||
| .then((result) => { | ||||||
| console.debug('saved contact', result); | ||||||
|
|
||||||
|
|
@@ -345,6 +364,11 @@ export class ContactsEditComponent implements OnInit, OnDestroy, AfterViewInit { | |||||
| this.router.navigate(['/contacts', result.docId]); | ||||||
| }) | ||||||
| .catch((err) => { | ||||||
| if (err instanceof DuplicatesFoundError){ | ||||||
| this.duplicates = err.duplicates; | ||||||
| err = Error(err.message); | ||||||
|
There was a problem hiding this comment. question: why do we need to reassign There was a problem hiding this comment. I believe the idea was to "box" the value and omit dumping the duplicate data in console. There was a problem hiding this comment. Destructive reassignment is usually a code smell (the go community heard what I said). You don't know who else wants to use a variable or why and now they have to work around your choices. There was a problem hiding this comment.
👍 Thanks for the context. I understand where you are coming from here now. FWIW, I don't know that we really have any solid patterns around "destructive reassignment" in the CHT code-base. 😅 So, I am happy to go with which ever way seems most clean to you. |
||||||
| } | ||||||
|
|
||||||
| console.error('Error submitting form data', err); | ||||||
|
|
||||||
| this.globalActions.setEnketoSavingStatus(false); | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you preferred this
levenshteinpackage? I know that metrics are not everything, but I was looking at fastest-levenshtein and it seemed to have the following benefits:levenshtein...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the
levenshteinlib author got it right the first time, why mess with perfection 😆 (just kidding).You raise a valid point, and I've updated the implementation to use
fastest-levenshtein. Since this calculation will see significant traffic, especially on lower-spec devices, prioritizing performance makes sense.Interestingly,
fast-levenshteinusesfastest-levenshteinunder the hood - yet ironically has more downloads. I'm not sure how much weight download count carries here.The key difference is that the
levenshteinlibrary does not depend on any other library, whereasfastest-levenshteindepends onlevenshtein-edit-distance, one of the libraries it compares itself to in its benchmarking (specifically version "2.0.5"cli.js).Another difference would be that the levenshtein lib allows you to pretty print the distance table, but that's just nice to have 🙂.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was looking at
fast-levenshteinas well. It seemed like it maybe just added some extra functionality on top offastest-levenshtein. Ultimately, I agree we want to optimize for performance (especially on low-spec devices). Additionally, we should be sensitive about adding more kb to our app bundle size. 😬 IMHO it seems likefastest-levenshteincan provide a good balance for us and a good place to start. However, as you noted in a previous comment, we may decide in the future that the locale sensitive comparisons fromfast-levenshteinare needed for non-English use cases...