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 signature verification link #205

Merged
merged 3 commits into from
Jan 13, 2020
Merged

fix signature verification link #205

merged 3 commits into from
Jan 13, 2020

Conversation

adridadou
Copy link
Member

@adridadou adridadou commented Jan 9, 2020

This PR does 2 things:

  • remove an unused class that is in conflict with another one in the main project
  • fix the URL generated to verify a signature

This change is Reviewable

Copy link
Contributor

@craig-openlaw craig-openlaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to make a regression test for this with our current test infrastructure?

Copy link
Contributor

@fforbeck fforbeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I got the idea here. Do we need to build the link to the signature proof on openlaw core? Because it depends on openlaw app context. Could we just instantiate the link on openlaw app with the proper data? I mean, for each change in the app context it will require a new change in the core lib to fix the context.

@adridadou
Copy link
Member Author

The issue here is that this is part of the rendering process, i.e. we need to generate this link within the rendering process.

So for now, this is the solution we have found. We can def. create an issue to extract that (or actually just put a pin on it for when we're going to work more on the signature process) so we don't have this issue anymore.

I agree this is not an ideal architecture but this is what works right now

Copy link
Contributor

@fforbeck fforbeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I just raised the new ticket #207

@adridadou adridadou merged commit ff19d57 into master Jan 13, 2020
@adridadou adridadou deleted the multiple-instances branch January 13, 2020 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants