lifterlms icon indicating copy to clipboard operation
lifterlms copied to clipboard

Bug in export students report

Open alaa-alshamy opened this issue 3 years ago • 4 comments

Reproduction Steps

  • Install Lifterlms
  • Switch the WordPress language to non latin language like Arabic as me
  • Make sure the lifterlms translation is pulled
  • Open the Report section in the Lifterlms dashboard and try to export the Students report

Expected Behavior

  • Should work and install the csv file as expected

Actual Behavior

  • The file download fail and show server problem Screenshot from 2021-02-22 10-44-14

Bug Info

After deep looking i figured the reason of this bug, the generated file name has a URL encoded characters because it uses the title of the table we export, i mean here 'students' but because we pulled the arabic translations the translation text of 'students' will be used, see the image below: Screenshot from 2021-02-22 10-33-59

and then when we request the file in the browser the server will evaluate the URL encoded characters so it will not match the file name so i need to URL encode the word 'الطلاب' (students) twice then use it in the request to get it work

maybe it's better to not use the title of the table at all and just use the id of the table

https://github.com/gocodebox/lifterlms/blob/trunk/includes/abstracts/llms.abstract.exportable.admin.table.php#L75

Note: other report section like courses is working as expected because it's not using the table title so the file name is always begin with 'llms-courses'

Error Messages / Logs

  • Include any relevant error messages or log files
<!-- Paste error logs / backtraces below this line -->

System and Environment Information

System Report


This issue has be recreated:

  • [x] Locally
  • [x] On a staging site
  • [x] On a production website
  • [x] With only LifterLMS and a default theme

Browser, Device, and Operating System Information

  • Browser name and version
  • Operating System name and version
  • Device name and version (if applicable)

alaa-alshamy avatar Feb 22 '21 08:02 alaa-alshamy

for anyone who has this problem the possible solutions for now:

  • Without any edit just get the file link and double encode the translation of word 'students' and request it again
  • Or Remove the translation of the word 'students' from the LifterLMS by for example Loco translate plugin
  • Or Use the following filter code as a plugin or in your active theme
function fix_export_students_report() {
	return 'students';
}
add_filter( 'llms_table_get_students_export_title', 'fix_export_students_report');

alaa-alshamy avatar Feb 22 '21 09:02 alaa-alshamy

@alaa-alshamy thanks for reporting this. add_filter( 'llms_table_get_student_export_file_name', 'sanitize_file_name' ); fix the issue as well?

eri-trabiccolo avatar Feb 22 '21 09:02 eri-trabiccolo

@eri-trabiccolo Yep it's possible but with this filter you need to define all parts of the file name not only the table title part so i preferred the filter 'llms_table_get_students_export_title' instead

as we can see here in this line: https://github.com/gocodebox/lifterlms/blob/trunk/includes/abstracts/llms.abstract.exportable.admin.table.php#L232

there's another parts to make the file name unique and hard to be expected i think by using current_time and wp_generate_password

alaa-alshamy avatar Feb 22 '21 09:02 alaa-alshamy

You're probably right, given how file name is generated we could just replace sanitize_title here: https://github.com/gocodebox/lifterlms/blob/4.16.0/includes/abstracts/llms.abstract.exportable.admin.table.php#L232 with sanitize_file_name. Assuming that either current_time( 'Y-m-d' ) and wp_generate_password( 8, false, false ) do not generate not suitable characters.

eri-trabiccolo avatar Feb 22 '21 10:02 eri-trabiccolo