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

Scix 373 Email Notification (formerly myADS) #449

Merged
merged 26 commits into from
Apr 17, 2024

Conversation

shinyichen
Copy link
Member

Screen.Recording.2024-03-19.at.11.40.33.AM.mov

Copy link

codecov bot commented Mar 19, 2024

Codecov Report

Attention: Patch coverage is 43.54244% with 1377 lines in your changes are missing coverage. Please review.

Project coverage is 47.3%. Comparing base (b077706) to head (b7e0424).
Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #449     +/-   ##
========================================
- Coverage    47.3%   47.3%   -0.0%     
========================================
  Files         413     425     +12     
  Lines       41996   44298   +2302     
  Branches      597     596      -1     
========================================
+ Hits        19861   20916   +1055     
- Misses      22124   23371   +1247     
  Partials       11      11             
Files Coverage Δ
src/api/models.ts 100.0% <100.0%> (ø)
src/api/vault/types.ts 100.0% <100.0%> (ø)
src/components/EmailNotifications/ArxivModel.ts 100.0% <100.0%> (ø)
src/components/EmailNotifications/index.ts 100.0% <100.0%> (ø)
src/components/TimeSince/index.ts 100.0% <100.0%> (ø)
src/components/index.ts 100.0% <100.0%> (ø)
src/mocks/handlers.ts 100.0% <100.0%> (ø)
src/components/Libraries/LibraryListTable.tsx 66.3% <50.0%> (+3.1%) ⬆️
src/lib/useColorModeColors.ts 64.0% <60.0%> (-0.3%) ⬇️
src/components/TimeSince/TimeSince.tsx 17.4% <17.4%> (ø)
... and 10 more

Copy link
Member

@thostetler thostetler left a comment

Choose a reason for hiding this comment

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

This functionally looks good to me. I have a small concern about cleaning user input and I think we should improve the error handling a bit.

{
onSettled(data, error) {
if (error) {
toast({ status: 'error', title: 'Error', description: parseAPIError(error) });
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be handled slightly differently. Currently the description is not actually getting parsed because it's path is different than the other microservices: (response/data/msg) instead of message -- so we see:
image
If I fix the parsing:
image

The error message is worse:
image

I think, on error here we should probably check if it's syntax issue and alert on the form instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

@thostetler Please see my last commit. I added a syntax check.

@shinyichen shinyichen force-pushed the SCIX-373-myads branch 2 times, most recently from d853dc3 to fb7cea6 Compare April 5, 2024 23:18
@shinyichen shinyichen force-pushed the SCIX-373-myads branch 2 times, most recently from 11af07d to aeea656 Compare April 12, 2024 21:19
@shinyichen
Copy link
Member Author

@thostetler Updated to check the validity of keyword. Let me know if it looks good to go.

Copy link
Member

@thostetler thostetler left a comment

Choose a reason for hiding this comment

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

This is looking good to me

@thostetler thostetler merged commit 7ff96a1 into adsabs:master Apr 17, 2024
3 of 4 checks passed
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.

None yet

2 participants