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

Fix batch transaction export on D8/9 #19761

Merged
merged 1 commit into from
Mar 11, 2021

Conversation

MegaphoneJon
Copy link
Contributor

https://lab.civicrm.org/dev/financial/-/issues/169

Overview

2 of the 3 methods to export a batch on D8/9 fail because jquery.redirect.min.js can't be found. Full replication details on the ticket.

Before

No response when you press "Export".

After

Export proceeds. Also saves an unnecessary modal from popping up.

Technical Details

The Dev console shows the error - "jquery.redirect.min.js" can't be found. This has to do with how the Resource URLs are split into core and packages inside /libraries/civicrm. It's impossible to call jquery.redirect.min.js.

Comments

  • This is the last reference in the Civi code to jquery.redirect.min.js. Perhaps this jQuery plugin can be removed/deprecated?
  • This UI has always been a bit flaky. If you click Transactions on the Open Batches screen, the Export button will bring you to the next screen. The two methods I outlined on the PR bring up an extra modal dialog, the options of which are repeated on the next screen. So rather than fix the bug, I removed the modal altogether.

@civibot
Copy link

civibot bot commented Mar 8, 2021

(Standard links)

@civibot civibot bot added the master label Mar 8, 2021
@seamuslee001
Copy link
Contributor

@MegaphoneJon would it not be easier to just update the addScriptFile to reference the packages directory as per

->addScriptFile('civicrm.packages', 'backbone/json2.js', 100, 'html-header', FALSE)

@MegaphoneJon
Copy link
Contributor Author

@seamuslee001 Perhaps. However, two screens that present the same set of options isn't great UX. I'd rather do the extra work and remove this altogether.

@demeritcowboy
Copy link
Contributor

I can reproduce the problem and agree it's probably better to remove the drupal resources factor. I can test this this week.

If you click Transactions on the Open Batches screen, the Export button will bring you to the next screen.

Not sure why this is a problem since you need to choose the export format somehow but will try some runs.

@demeritcowboy
Copy link
Contributor

demeritcowboy commented Mar 10, 2021

Overall it looks good just maybe one minor glitch and not sure yet if it's my setup. Tested with drupal 9 and drupal 7.

And in my comment earlier I think I misread what you were saying - the "Flaky" wasn't referring to just the next sentence in that paragraph it was referring to the whole paragraph. All good.

  • General standards
    • [r-explain] PASS
    • [r-user] PASS
    • [r-doc] PASS
    • [r-run] Issue
      • On drupal 9 the Cancel button no longer works after you choose the export format and export a batch so you're stuck on that screen. It does still work on drupal 7. I'll take a closer look but maybe you already know the answer.
      • Not related to the PR, but there's a buttonrama issue that is different from the previous point, where what's supposed to happen is after you click Export the Cancel button text is supposed to change to the word Done. To do that on a button is a different method than on the old input. https://lab.civicrm.org/dev/financial/-/issues/170
  • Developer standards
    • [r-tech] PASS
    • [r-code] PASS
    • [r-maint] ?
    • [r-test] PASS

@demeritcowboy
Copy link
Contributor

demeritcowboy commented Mar 11, 2021

Ok I see what the first Cancel button issue is. Drupal 9 has an equivalent of civi's submitOnce in web/core/misc/form.js: data-drupal-form-submit-last. This leads to e.preventDefault(); which makes the button stop working after you've already exported.

An easy way to see this is when you're at this point just open browser dev tools and remove the data-drupal-form-submit-last from the <form> element. Now the cancel button works again.

So I think this PR is mergeable as-is and the cancel button freeze could be dealt with separately TBD.

@MegaphoneJon
Copy link
Contributor Author

MegaphoneJon commented Mar 11, 2021 via email

@seamuslee001
Copy link
Contributor

merging as per @demeritcowboy review

@seamuslee001 seamuslee001 merged commit 3e42836 into civicrm:master Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants