Skip to content
This repository has been archived by the owner on Jul 9, 2020. It is now read-only.

Improved config validation #178

Merged
merged 3 commits into from Jun 27, 2020
Merged

Improved config validation #178

merged 3 commits into from Jun 27, 2020

Conversation

NoumbissiValere
Copy link
Contributor

No description provided.

@coveralls
Copy link

coveralls commented Apr 9, 2020

Coverage Status

Coverage increased (+0.0008%) to 99.915% when pulling abbd235 on improved-config-validation into ff7065f on master.

@NoumbissiValere NoumbissiValere deleted the improved-config-validation branch April 9, 2020 00:57
@NoumbissiValere NoumbissiValere restored the improved-config-validation branch April 9, 2020 01:10
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

First impression looks good, I will test it asap.
In the meanwhile, please can you reduce the number of commits? I think one commit would be enough for this set of changes.

@NoumbissiValere
Copy link
Contributor Author

ok @nemesisdesign I will squash the commits

@NoumbissiValere NoumbissiValere force-pushed the improved-config-validation branch 3 times, most recently from 424d212 to 33d0e89 Compare April 15, 2020 09:41
@NoumbissiValere
Copy link
Contributor Author

@nemesisdesign This PR looks ready. sorry for the delay

@okraits okraits self-assigned this Apr 24, 2020
@atb00ker atb00ker force-pushed the improved-config-validation branch 11 times, most recently from df33aeb to c1554ac Compare May 15, 2020 15:19
Copy link
Member

@atb00ker atb00ker left a comment

Choose a reason for hiding this comment

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

Two questions regarding this pull request below:

django_netjsonconfig/tests/test_template.py Show resolved Hide resolved
django_netjsonconfig/base/template.py Outdated Show resolved Hide resolved
@atb00ker atb00ker added this to In progress in [GSoC20] Merge modules via automation May 15, 2020
@atb00ker atb00ker moved this from In progress to Ready for review in [GSoC20] Merge modules May 15, 2020
@atb00ker atb00ker moved this from Ready for review to Open Pull-Requests in [GSoC20] Merge modules May 15, 2020
@atb00ker atb00ker force-pushed the improved-config-validation branch from c1554ac to 5a0b141 Compare May 16, 2020 19:17
@atb00ker atb00ker moved this from Open Pull-Requests to Ready for review in [GSoC20] Merge modules May 16, 2020
@atb00ker
Copy link
Member

atb00ker commented Jun 8, 2020

@NoumbissiValere

This issue was caused by https://github.com/openwisp/django-netjsonconfig/blob/master/django_netjsonconfig/base/template.py#L133
I removed it and replaced it with

This doesn't solve the problem though, It just ignores the variable! :-(
I think this needs to be handled at the time of save, I'll look into it as well! 😄

    def get_context(self):
        context = copy(self.default_values) or {}
        context.update(super().get_context())
        return context

solved it.

P.P.S: @NoumbissiValere can you please rebase to upstream/improved-config-validation before pushing! 😄

@atb00ker atb00ker force-pushed the improved-config-validation branch from 6cc61a3 to e4a6d50 Compare June 8, 2020 09:22
@NoumbissiValere
Copy link
Contributor Author

Sure 😄 I will do further testing to make sure everything is ok with the changes I made and push

@atb00ker atb00ker force-pushed the improved-config-validation branch from e4a6d50 to 85378bc Compare June 8, 2020 12:27
@atb00ker
Copy link
Member

atb00ker commented Jun 8, 2020

@nemesisdesign
Validation acts weirdly 1 & Validation acts weirdly 2 happens because the default values json used was not valid, which brings an issue that the user should be informed when the json is not valid, worked on that! 😄

@NoumbissiValere
Copy link
Contributor Author

@nemesisdesign
Validation acts weirdly 1 & Validation acts weirdly 2 happens because the default values json used was not valid, which brings an issue that the user should be informed when the json is not valid, worked on that!

@atb00ker the validation acts weirdly 2 doesn't depend on the default_values json it is a functionality which should work even without the default values. I have tested thoroughly but could still see the error hints. so we still need to look into that

@NoumbissiValere
Copy link
Contributor Author

@atb00ker Please when you have time, could you test that error hints are displayed when you inside a buggy template in advanced mode? I don't see anything but inspecting shows they are there. this happens on my browser across other modules which uses django-netjsonconfig. below is a screenshot of the output i get from openwisp-controller which is thesame with the output i get from this branch on django-netjsonconfig.
Screenshot from 2020-06-09 07-22-45

I don't know if the error is because of my browser or it is a general problem. could you test it and give me feedback? thanks 😄

@atb00ker
Copy link
Member

Not sure why it isn't shown on your end, on my end it's working as expected. (Can you try refreshing your browser cache?)

@NoumbissiValere
Copy link
Contributor Author

I had not only refresh but also did a hard reload. But since it's working on your end it's ok. I will work on my browser. Thank you for taking our time to test @atb00ker 😊
@nemesisdesign could you review it now? All the changes requested have been done.

@atb00ker atb00ker force-pushed the improved-config-validation branch 3 times, most recently from 14756fe to 6a17080 Compare June 11, 2020 13:05
Fixed the issue by removing uneccessary work which @okraits began by
doing after he extracted the gsoc2019 branch.

Fixes #175
README.rst Outdated
@@ -50,6 +50,40 @@ Current features
* **template tags**: tag templates to automate different types of auto-configurations (eg: mesh, WDS, 4G)
* **simple HTTP resources**: allow devices to automatically download configuration updates
* **VPN management**: easily create VPN servers and clients
* **Template variables**: makes it possible to declare context (configuration variables) in template configuration while setting the default values for these declared context in the default values field to bypass validation.
Copy link
Member

@atb00ker atb00ker Jun 11, 2020

Choose a reason for hiding this comment

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

@NoumbissiValere
Should we move the focus of this line from bypass validation to allows setting variable; i.e from user's perspective?

P.S: I removed some changes that I made wouldn't required now. Please rebase again. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok @atb00ker 😄

@atb00ker atb00ker moved this from Open Pull-Requests to Ready for review in [GSoC20] Merge modules Jun 11, 2020
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Great work @NoumbissiValere and thank you @atb00ker for your review and support.
This is what I call team work! 👍 💪

Finally merging this! 🎉 🎉
Thank you for your patience and persitence! 🙏

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants