Custom Wizard should not override Application Route

merefield · July 11, 2023, 5:16pm

As discussed on Discord.

PR complete here and ready for review:

@Angus, note I’ve removed what I believe to be redundant code too, please review carefully

angus · July 11, 2023, 5:24pm

Could you please identify which code you think is redundant and why you think it’s redundant.

merefield · July 11, 2023, 5:26pm

Sure, np, this:

github.com

paviliondev/discourse-custom-wizard/blob/c4521b4b925d230780530bd7c4c249949f0d41f7/assets/javascripts/discourse/initializers/custom-wizard-edits.js.es6#L15


      
          
          export default {
            name: "custom-wizard-edits",
            initialize(container) {
              const siteSettings = container.lookup("service:site-settings");
          
              if (!siteSettings.custom_wizard_enabled) {
                return;
              }
          
              const existing = DiscourseURL.routeTo;
              DiscourseURL.routeTo = function (path, opts) {
                if (path && path.indexOf("/w/") > -1) {
                  return (window.location = path);
                }
                return existing.apply(this, [path, opts]);
              };
          
              withPluginApi("0.8.36", (api) => {
                api.modifyClass("component:d-navigation", {
                  pluginId: "custom-wizard",

Reason: I don’t understand why this override is needed (and all tests pass without it).

merefield · July 11, 2023, 7:28pm

btw, I tested this successfully in Production too using the “Users directed to wizard after start time” setting with my greatest invention yet, the “Boo!” Wizard:

Brilliant, huh?

But let me tell you, it makes for quite a shock

If you want a copy I’ll gladly provide the json …

angus · July 12, 2023, 7:09am

Please put that back in. It’s needed to ensure wizard redirects that get picked up by the DiscourseURL.routeTo helper (not all route transitions but some).

We don’t have any tests for that

Generally speaking leave things in if you don’t understand what they’re doing.

merefield · July 12, 2023, 7:24am

We need to be careful not to build technical debt.

We really need tests on things so we know what they do and it is implicitly documented.

That said I appreciate integration code is very hard to add tests for.

I will add back.

merefield · July 12, 2023, 12:20pm

@Angus this is ready for review again.

And to make things easier to see, I’ve restored the original file structure so the changes are more obvious.

merefield · July 14, 2023, 1:13pm

This issue should be closed now: