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

Show an informational warning before performing hierarchy operations #233

Merged
merged 37 commits into from
Dec 24, 2024

Conversation

kennsippell
Copy link
Member

@kennsippell kennsippell changed the base branch from main to 162-merge-contacts December 20, 2024 18:58
@binokaryg binokaryg requested a review from Copilot December 20, 2024 19:18

Choose a reason for hiding this comment

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

Copilot reviewed 3 out of 5 changed files in this pull request and generated no comments.

Files not reviewed (2)
  • src/liquid/components/manage_hierarchy_warning.html: Language not supported
  • src/liquid/place/manage_hierarchy_form.html: Language not supported
@binokaryg binokaryg requested a review from Copilot December 20, 2024 19:42
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 6 changed files in this pull request and generated 1 comment.

Files not reviewed (2)
  • src/liquid/components/manage_hierarchy_warning.html: Language not supported
  • src/liquid/place/manage_hierarchy_form.html: Language not supported
Comments suppressed due to low confidence (2)

src/lib/manage-hierarchy.ts:88

  • [nitpick] The describeDateTime function returns a hyphen ('-') for invalid or negative dates, which might be confusing.
return '-';

src/lib/manage-hierarchy.ts:67

  • [nitpick] The variable name syncBelowThreshold is ambiguous and could be more descriptive.
userIsActive: syncBelowThreshold && lastSyncDescription !== '-';

jobData,
};
function diffNowInDays(dateTime: DateTime): number {
return -(dateTime?.diffNow('days')?.days || 0);
Copy link
Preview

Copilot AI Dec 20, 2024

Choose a reason for hiding this comment

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

The diffNowInDays function returns negative values, which is counterintuitive. It should return positive values for days in the past and zero or more for future dates.

Suggested change
return -(dateTime?.diffNow('days')?.days || 0);
return dateTime?.diffNow('days')?.days || 0;

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
Copy link
Member Author

Choose a reason for hiding this comment

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

Actually the Luxon library's DateTime.diffNow function returns negatives and that negative makes them positive

@@ -134,6 +135,20 @@ export class ChtApi {
return this.axiosInstance.post(url, deactivationPayload);
};

countContactsUnderPlace = async (docId: string): Promise<number> => {
const url = `medic/_design/medic/_view/contacts_by_depth`;
console.log('axios.get', url);
Copy link
Member

Choose a reason for hiding this comment

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

Why are we logging a constant? Do we wish to keep it for a while?

Copy link
Member Author

Choose a reason for hiding this comment

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

I personally like seeing when the server makes requests to CHT, so all outbound requests from this cht-api library are logged. Fred has also indicated that he didn't like this pattern, but for now I'll stay consistent with the file and we can remove everywhere if we wish to remove.

Base automatically changed from 162-merge-contacts to main December 23, 2024 07:39
export function hierarchyViewModel(action: string, contactType: ContactType) {
const parentTypeName = contactType.hierarchy.find(h => h.level === 1)?.contact_type;
if (!parentTypeName) {
throw Error('parent type name');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
throw Error('parent type name');
throw Error('Parent type name not found in contact type hierarchy');

Copy link
Member

@binokaryg binokaryg left a comment

Choose a reason for hiding this comment

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

Approving with a few optional suggestions.

@kennsippell kennsippell merged commit 4ffd976 into main Dec 24, 2024
1 check passed
@kennsippell kennsippell deleted the 232-delete-warning branch December 24, 2024 01:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants