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

Harvest #585

Merged
merged 6 commits into from Jan 4, 2022
Merged

Harvest #585

merged 6 commits into from Jan 4, 2022

Conversation

eluce2
Copy link
Contributor

@eluce2 eluce2 commented Dec 31, 2021

Description

  • Fix: Update SWR package to 1.1.2 (handles window.requestAnimationFrame bug)
  • Fix: Harvest admins are now limited to only view their own time entries, even if their API creds allow them to view/edit other user's timers in their company.
  • New: Support for viewing timers on other days (see below)
  • New: Add user preference to control how time entries are sorted
  • Improvement: Remove brackets in project picker if no code is present (fixes [Extension Bug] Harvest - @eluce2  #538)
  • Improvement: Project picker now groups projects by client (more similar the Harvest app)
  • Improvement: Now uses new alert dialog for deleting time entry confirmation
  • Improvement: Total hours for a given day are shown in the header of the list of time entries

NOTE: The new feature that allows editing entries on other days is incomplete, but until there is a Date only form field component, I can't get it to function correctly. Expected behavior is that you would navigate to a different day, then choose the "New Timer" action and the date field would be prefilled to the day that you had navigated to. Due to timezone differences, if I put any date in as the default value, it displays as 6 pm on the previous day. To me, this behavior is worse than not pre-setting the date at all, so I've disabled that part of the feature for now. However, if the date field is left blank, the API will still create the time entry on the correct date this is just not made clear to the user in the form.

Also, please note it was an intentional decision to change the navigation header when switching days, as otherwise there would be no indication of which is your current day if there are no time entires on that day.

Type of change

Bug fix / improvement for my extension

Screencast

https://cln.sh/meou3X

Checklist

Copy link
Contributor

@unnamedd unnamedd left a comment

Choose a reason for hiding this comment

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

Hey @eluce2, would you please do not modify other extensions?
I saw you are making changes on my Script Commands Store extension and also in the Two Factor Authentication as well.

Please, rollback those changes.

Tip: when you develop you extension, open on VSCode only the folder of your extension, if you open the extensions root folder and apply a formatter, it might make unwanted changes on other files.

@eluce2
Copy link
Contributor Author

eluce2 commented Jan 1, 2022

@unnamedd Sorry about that, this new commit should only effect my own extension. Thanks for the tip, I'll be sure to use that in my future development!

Copy link
Collaborator

@pernielsentikaer pernielsentikaer left a comment

Choose a reason for hiding this comment

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

Hi @eluce2 👋

It sounds like some nice additions to the extension 💪

  • In the README recognize is spelled wrong 😄
  • There is no gracefully handling when token are wrong, can you also add this?

HB7KoBFawfV0f1k7

Request a new review when you are ready, feel free to contact me here or at Slack if you have any questions.

@pernielsentikaer pernielsentikaer self-assigned this Jan 2, 2022
@pernielsentikaer pernielsentikaer added new extension Label for PRs with new extensions extension fix / improvement Label for PRs with extension's fix improvements and removed new extension Label for PRs with new extensions labels Jan 2, 2022
Copy link
Collaborator

@pernielsentikaer pernielsentikaer left a comment

Choose a reason for hiding this comment

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

hi @eluce2 👋

Did you push it correct? It seems like I'm still facing the same hard error 😄

IgFuJiUwMmAyMGwJ

@eluce2
Copy link
Contributor Author

eluce2 commented Jan 3, 2022

@pernielsentikaer So that looks like a different error than "invalid API token". At first, I was unable to replicate, but that's because I've been testing with the node production environment. Could this be something polyfilled by the Raycast Dev API, or do I need to adjust my code? Again, you won't get that error when it's running in production.

@pernielsentikaer
Copy link
Collaborator

Hi @eluce2 👋

Just to be sure once more, sorry, If I get the extension from Store right now, this is the error you have fixes, yes? (The one with wrong credentials)

4WfWGmgTIAzNySEh

@eluce2
Copy link
Contributor Author

eluce2 commented Jan 3, 2022

Yes, I'm trapping for the 401 HTTP response now in the latest push, and showing a generic error for any other non-success HTTP statuses

Copy link
Collaborator

@pernielsentikaer pernielsentikaer left a comment

Choose a reason for hiding this comment

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

hi @eluce2 👋

Thanks for the update. I'm going to accept it and then will my colleague do a final review before it's finally merged 😄

Comment on lines 73 to 83
if (error.isAxiosError) {
if (error.response?.status === 401) {
await showToast(
ToastStyle.Failure,
"Invalid Token",
"Your API token or Account ID is invalid. Go to Raycast Preferences to update it."
);
}
} else {
await showToast(ToastStyle.Failure, "Unknown Error", "Could not fetch time entries");
}
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it be

Suggested change
if (error.isAxiosError) {
if (error.response?.status === 401) {
await showToast(
ToastStyle.Failure,
"Invalid Token",
"Your API token or Account ID is invalid. Go to Raycast Preferences to update it."
);
}
} else {
await showToast(ToastStyle.Failure, "Unknown Error", "Could not fetch time entries");
}
if (error.isAxiosError) {
if (error.response?.status === 401) {
await showToast(
ToastStyle.Failure,
"Invalid Token",
"Your API token or Account ID is invalid. Go to Raycast Preferences to update it."
);
} else {
await showToast(ToastStyle.Failure, "Unknown Error", "Could not fetch time entries");
}
} else {
await showToast(ToastStyle.Failure, "Unknown Error", "Could not fetch time entries");
}

so that a 500 or whatever also shows a toast? We might want to show the error message as well (if any, I'm not familiar with the API)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good catch. I'll add that

Copy link
Member

@mathieudutour mathieudutour left a comment

Choose a reason for hiding this comment

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

Thanks!

@mathieudutour mathieudutour merged commit 374cddf into raycast:main Jan 4, 2022
@raycastbot
Copy link
Collaborator

Published to the Raycast Store:
https://raycast.com/eluce2/harvest

FezVrasta pushed a commit to FezVrasta/extensions that referenced this pull request Mar 23, 2022
* revert rebase

* graceful error handling of invalid API token

* readme spellcheck fix

* fix typechecking of error clause

* Update extensions/harvest/src/listTimeEntries.tsx

Co-authored-by: Mathieu Dutour <mathieu@dutour.me>

* add else case to support other HTTP statuses

Co-authored-by: Mathieu Dutour <mathieu@dutour.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension fix / improvement Label for PRs with extension's fix improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Extension Bug] Harvest - @eluce2
5 participants