search

accordproject / cicero-template-library

Accord Project Template Library
https://templates.accordproject.org
Apache License 2.0
80 stars 93 forks source link

fix(njk): replacement of link for time model - #245 #385

Closed Aniruddha-Shriwant closed 3 years ago

Aniruddha-Shriwant commented 3 years ago

Signed-off-by: Aniruddha Shriwant aniruddhashriwant@gmail.com

Closes #245

Changes

Author Checklist

Aniruddha-Shriwant commented 3 years ago

I had fixed the bug by replacing this line : <li><a href="{{uri | replace(".cto", ".html")}}">{{ns}}</a></li> with this <li><a href="{{uri | replace("v2.0/time", "time@0.2.0") | replace(".cto", ".html")}}">{{ns}}</a></li>

But when I run npm run build and opens some HTML files which were having the bug, I noticed that the bug is still present in them. Hence for testing purpose I incremented their version from package.json and then run the npm run build command again, as the new files for that particular template are generated, by opening those files in browser I noticed that the bug was resolved...

So my question is should I increment the template version which are having the bug (I had already listed which templates are having the bug here) Or is there any other solution for that?

Originally posted by @Aniruddha-Shriwant in https://github.com/accordproject/cicero-template-library/issues/245#issuecomment-803464470

Aniruddha-Shriwant commented 3 years ago

But when I run npm run build and opens some HTML files which were having the bug, I noticed that the bug is still present in them.

You can see here that the bug is still present

Steps to reproduce the behavior:

  1. Go to https://deploy-preview-385--templates-accordproject.netlify.app/fixed-interests@0.5.2.html
  2. Click on org.accordproject.time.* in the model dependencies
  3. See error
Aniruddha-Shriwant commented 3 years ago

Hence for testing purpose I incremented their version from package.json and then run the npm run build command again, as the new files for that particular template are generated, by opening those files in browser I noticed that the bug was resolved...

image

As this was pointing to https://models.accordproject.org/time@0.2.0.html the bug was resolved...

mttrbrts commented 3 years ago

Thanks for the investigation @Aniruddha-Shriwant .

Rebuilding templates is sensitive and should be avoided wherever possible. This is because the templates were built with older versions of Cicero, and we need to retain the static build output where possible.

Instead of adding custom logic to the build process, I suggest adding a redirect to the Netlify configuration.

This will help us to better centralise broken links when assets have been moved.

The docs for Netlify redirects is here https://docs.netlify.com/routing/redirects/

Aniruddha-Shriwant commented 3 years ago

Ignore the commits I will make/had made, I'm just testing Netlify configurations and after success will make a new PR with clean commits

Aniruddha-Shriwant commented 3 years ago

@mttrbrts I had read this https://docs.netlify.com/routing/redirects/ and made some configurations for testing _redirects and netlify.toml but they were not working... I think I'm making some mistake don't know where :) Can you help me with these? Also, apologize for wasting netlify deploy minutes (~3minutes per build)

mttrbrts commented 3 years ago

@mttrbrts I had read this https://docs.netlify.com/routing/redirects/ and made some configurations for testing _redirects and netlify.toml but they were not working...

Ah ha! I see the problem. models.accordproject.org are hosted by https://github.com/accordproject/models/ So this issue needs to be fixed there. Sorry for the confusion!

You'll probably need a config that looks like this ...

/v2.0/time.html /time@0.2.0.html

Also, apologize for wasting netlify deploy minutes (~3minutes per build)

No problem at all!

Aniruddha-Shriwant commented 3 years ago

Ah ha! I see the problem. models.accordproject.org are hosted by https://github.com/accordproject/models/ So this issue needs to be fixed there. Sorry for the confusion!

Ohh! So @mttrbrts, should I open an Issue for that in models and make PR for this, which will have _redirects file in root folder just like it has _headers(https://github.com/accordproject/models/blob/master/_headers)

You'll probably need a config that looks like this ...

/v2.0/time.html /time@0.2.0.html

Yeah thanks for this I was just testing if the redirect was working hence tried /acceptance-of-delivery@0.14.1.html /car-rental-tr@0.11.0.html in _redirect

Aniruddha-Shriwant commented 3 years ago

So should I close this PR as this will be of no use now :)

Aniruddha-Shriwant commented 3 years ago

Ohh! So @mttrbrts, should I open an Issue for that in models and make PR for this, which will have _redirects file in root folder just like it has _headers(https://github.com/accordproject/models/blob/master/_headers)

I will also copy _redirects like this

https://github.com/accordproject/models/blob/c614455d42d61cd3c18f30a6d60b7fe38f9a375c/build.js#L224