search

moodleou / moodle-report_customsql

A Moodle report plugin that lets you easily create simple reports that can be expressed as a single SQL query
48 stars 100 forks source link

Fix #114 - return empty csv file (with headers) when no results. #122

Closed danmarsden closed 11 months ago

danmarsden commented 1 year ago

here's a method that allows us to use the plugins normal methods for printing columns in a report by adding a record of null values to the report when the main query is empty.

Hopefully it makes sense - seems to work cross db for me but would be good to test further - I'll see if we can get some other folks to test too.

timhunt commented 1 year ago

Why do you want to do this? What have you done. Sure, I am capable of RTC, but it would be easier to review this request if you explain.

danmarsden commented 1 year ago

Hey Tim, the 'reason' for this is covered in issue #114 is that what you're asking?

timhunt commented 1 year ago

That is what I was asking.

I don't like the proposed fix.

I understand why you want to chagne the behaviour when used as a web service, but I don't think we should change other cases. Therefore, why not change just the web service code, and ensure the header row is always output in PHP, rather than messing with the SQL?

If this behaviour is important enough to implement, it is important enough to have an automated test.

Alos, not really your problem, but if you could add a separat commit to fix the CI build (I think it just needs a DB version bump) then that would save me time.

danmarsden commented 1 year ago

Happy to look at adding tests, but.. any idea how to get the column names without using sql like this? If the query is something like 'select * from {table}' we have to execute the sql to get the list of columns.. and moodle doesn't have a show columns type api that let's us get a list if columns based on some sql.. it has to return rows.

timhunt commented 1 year ago

Oh yes. I was forgetting how this work. Too may things going on.

So, the issue is, if the report is being shown on-screen, we don't want the no rows behaviour to change. (Worth a Behat test to verify the current behaviour)

And, if this is a scheduled report, it is a feature that a file is only saved on disc if the report returned any data.

So, we only want the 'create empty file' behaviour in the web service case (that is, when the call to report_customsql_generate_csv comes from report_customsql_pluginfile in lib.php).

I agree it is hard to work out how to test the download behaviour. Possibly a custom Behat step like Then downloading customsql report "Query name" returns a file with headers "frog,toad". You can probably use behat_general::download_file_from_link to help implement that.

danmarsden commented 1 year ago

Thanks Tim - when I first initially wrote that SQL to add the extra null row I didn't like it either but I couldn't find a better way to do it! - you're right - it shouldn't be changing the normal view of the report (I forgot that the same function was used to print the report on the view page) - I've added some code to make that only happen on the pluginfile call. I'll look to see what extra behat tests I can add later too. (I've also adjusted the sql slightly - my earlier tests on mysql used a slightly different alias and I'd also forgotten the word "ignore" was a reserved word in mysql.)

timhunt commented 1 year ago

That is looking good now Dan. Just two minor quibbles which I would like to see fixed before I merge this.

danmarsden commented 1 year ago

I assume you got a phonecall just after posting that... or you mean the missing behat tests? - or you have comments still in a draft form... or the quibbles are because you're reviewing code way too late in the night... :-)

danmarsden commented 1 year ago

thanks Tim - basic changes implemented - haven't managed to write behat tests yet but will try to find some time today/tomorrow to add some in...

danmarsden commented 1 year ago

Hey Tim - I've just quickly dropped in a new behat test on that branch but I don't think the custom step is working quite right and I've obviously missed something/got a typo in there.... let me know if you spot the issue, otherwise I'll try to come back to this next week when I get some time.

danmarsden commented 1 year ago

that should be passing now... after trimming the bom and fixing a few typos...

abias commented 1 year ago

Hi Tim, hi Dan,

I would like to quickly confirm that the current patch in this PR solves the issue which I reported in #114 successfully.

danmarsden commented 1 year ago

Thanks @abias - @timhunt let me know if there's anything else you need here.

abias commented 1 year ago

Hi @timhunt ,

we are eagerly waiting for this PR to land in an official release so we can use it in one of our instanced officially. Is there any chance that you could have a final look and push this over the line?

Thanks in advance for your work!

brendanheywood commented 1 year ago

hi @timhunt

I've just tested this and it works great. The sql is weird but I can not see a better way of doing it without improving core, and I did propose this here but I still don't know how viable it is in every db driver:

https://tracker.moodle.org/browse/MDL-64387

Is there anything else blocking this from landing we can sort?

timhunt commented 11 months ago

Merged manually, with some small tweaks.

danmarsden commented 11 months ago

awesome - thanks heaps Tim! - hope you have a great break over Christmas! :-)

abias commented 7 months ago

@timhunt - Many thanks for merging this PR!

Would you mind to push out a new release to https://moodle.org/plugins/report_customsql/versions which contains this fix as well? Our customer who paid for this fix prefers to install official releases.

Many thanks for your time!