search

cfpb / regulations-site

(DEPRECATED) Web interface for viewing U.S. federal regulations and other regulatory information
Other
28 stars 43 forks source link

rewriting diff drop down update #614

Closed theresaanna closed 10 years ago

theresaanna commented 10 years ago

As you click through the TOC, the form action on the diff drop down changes to go to the right section if you choose to go into diff mode. If you are on an environment with an APP_PREFIX != '/', such as continuous, this broke.

cmc333333 commented 10 years ago

Is there a way to implement this without hard-coding the url? Could you just read the url, and swap out the last two components (section, version)?

theresaanna commented 10 years ago

So, that's more along the lines of the way it was. The problem there is that then I'm guessing at which url components are the section and version. Its a safer bet, but its certainly not foolproof. My logic is that the way they section and version end up in the form action url matches the way that the rest of the app gets them. So, if there's a problem here, there's probably a larger problem somewhere in the app.

Is it that "diff_redirect" is hard coded? We could certainly include that path similar to how we treat window.APP_PREFIX.

The original bug is that, the particular way I was splitting the existing url, I wasn't accounting for the APP_PREFIX, so things got knocked out of the spot they needed to be in.

cmc333333 commented 10 years ago

Right, "diff_redirect" is hard coded now, where it was not before.

Ideally, you should know which version and section you were on when changing them. Then you're just replacing strings. That's not what it did before, though, so let's just assume that it's the last two elements of the url are the section and version (as was done in the past).

The change you've proposed both makes that assumption (about the last two components being section and version) as well as forces the redirect url to include "diff_redirect", regardless of what the template wrote. That's less flexible than what we had before, which didn't force the latter.

theresaanna commented 10 years ago

Ok, I rewrote it in a way I think you'll like better.

While doing this, I also discovered a bug and a potential bug. findDiffVersion() had a logical flaw that I corrected and updated places where the versions are referred to to construct diff urls to remove a bug that was sometimes causing the newer version in the url to be undefined. I also removed some unnecessary complexity around using APP_PREFIX in the router. Its unnecessary because of the way its implemented in Django.

cmc333333 commented 10 years ago

Thanks, @theresaanna !