Add GSTR-I reports for B2B #277
Conversation
src/saveReportAsJson.js
Outdated
| export default async function makeJSON(data, savePath) { | ||
| fs.writeFile(savePath, data, (error) => { | ||
| if (error) throw error; | ||
| return shell.openPath(savePath); |
There was a problem hiding this comment.
Opening .json by default will in most cases just launch the text editor, raw json isn't something people generally would want to look at other than to verify that it did get saved.
I feel we should do one of these:
- Open the folder where it's saved.
- Show a toast saying that the file was saved at whatever path and clicking the toast opens the folder.
I don't know if we have UI for a toast, if we don't you can go with 1.
There was a problem hiding this comment.
Totaly 💯
Have identified a way to add toast too, but for now opening the directly seems like a good thing to do.
| { | ||
| label: 'Clear Filters', | ||
| type: 'secondary', | ||
| action: async report => { | ||
| await report.getReportData({}); | ||
| report.usedToReRender += 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
Any reason for removing the Clear Filter from this and General Ledger?
I noticed, prior to this PR this action wasn't implemented, I don't think we should remove it. If it doesn't work we can fix it.
There was a problem hiding this comment.
Yeah, we'll have to make it work I guess. I've removed them so that we can add them later as required.
There was a problem hiding this comment.
I've removed them so that we can add them later as required.
This doesn't make sense 😅
The problem I see is that someone'll end up implementing a whole other thing to add Clear Filter if we remove it. So since it's already there might as well leave it in. I don't mind pushing commits to fix it.
accounting/gst.js
Outdated
| message: _('Export failed!'), | ||
| description: _( | ||
| 'Report cannot be exported if company gst details are not configured' | ||
| ), |
There was a problem hiding this comment.
The title (message) can be "Export Failed", no need for punctuation in the title. Also full stop at the end of description.
p.s. I know this is very pedantic but these are almost zero cost improvements to maintain consistency
There was a problem hiding this comment.
@ankitsinghaniyaz I've added some comments, let me know once you're through with them, I'll re-review and we can merge.
I have yet to check report generation and calculations.
|
Sure @18alantom let me push these changes and ping you. |
24b85a2 to
fbe3c12
Compare
refactor: remove unnecessary prompt function
There was a problem hiding this comment.
LGTM now, still haven't cross checked the calculations, will do it when the report is complete, also made a few minor changes:
- Rebased onto
master - HSN code is not
required: 1 - Remove HSN code from
SalesInvoiceItem.jscause it's inRegionalChanges.js - Renamed Clear Filter to Reset Filters and moved it to report since it's common and existing implementation is dead code.
- Added group for Export, all exports should be under it
- Remove unnecessary Promise wrap around
showMessageDialog, reworded the message, action calls to receivegetReportDataso that filtered rows are fetched.
No description provided.