[FIX] issues #36,#37,#38 &39#40
Conversation
KKamaa
force-pushed
the
12537-update-chrome-firefox-fix
branch
from
f66dcd3 to
cc1fbc3
Compare
gfcapalbo
left a comment
gfcapalbo
left a comment
There was a problem hiding this comment.
nothing blocking, That's why i don't request changes.
- some naming could be more descriptive of what it does
- silent alert if alert function not fully loaded
- some more error management
- a url-db encoding would be better with a separator.
| * Download a CSV with current month timesheet rows. | ||
| */ | ||
| async downloadCurrentMonthTimesheets() { | ||
| const autoDownloadEnabled = await storage.get('auto_download_issue_timesheet', false); |
There was a problem hiding this comment.
A bit confusing. the function is downloadCurrentMonthTimesheets
returns early if auto_download_issue_timesheet is false
If auto_download_issue_timesheet should be called allow_download_issue_timesheet? there is nothing "auto" about it , its just allow/dissallow download.
| * Download a CSV for the currently timed item and current month. | ||
| */ | ||
| async downloadCurrentIssueTimesheet(issue) { | ||
| const today = new Date(); |
There was a problem hiding this comment.
I would add a tyr catch here to like you did in downloadCurrentMonthTimesheets to notify network errors or any other thing gone wrong.
|
|
||
| const customAlert = globalThis.alert && typeof globalThis.alert.show === 'function' | ||
| ? globalThis.alert | ||
| : null; |
There was a problem hiding this comment.
why? is alert not guaranteed to be present? it seems like there is a lot of defensive code, because we are not guaranteed globalThis.show fully loaded, wich is fine but
if it is not perhaps instead of null put in a minimal function with a basic message?
| <input | ||
| id="auto_download_timesheet_input" | ||
| type="checkbox" | ||
| t-att-checked="state.autoDownloadIssueTimesheet" |
There was a problem hiding this comment.
As i wrote above, "auto" doesn't look like the right name, should be "allow"
| <t | ||
| t-foreach="state.remotes" | ||
| t-as="remote" | ||
| t-key="remote.url + remote.database" |
There was a problem hiding this comment.
Maybe my only gripe. no separator.
This could cause problems in dividing URL and DB, a separator would be better.
Would not cause problems in 99% of urls and DBS but would be better with a separator string.
3 participants
Fixing issues #36 #37 #38 #39