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

Add option to hide SSID #56

Closed
martignoni opened this Issue Nov 9, 2018 · 7 comments

Comments

Projects
None yet
2 participants
@martignoni
Member

martignoni commented Nov 9, 2018

@martignoni martignoni added this to the 1.12.2 milestone Nov 9, 2018

@martignoni martignoni self-assigned this Nov 9, 2018

@martignoni

This comment has been minimized.

@adpe

This comment has been minimized.

adpe commented Nov 10, 2018

@martignoni: Can I contribute something or are you already working on it?

@martignoni

This comment has been minimized.

Member

martignoni commented Nov 10, 2018

I'm working on it, but as this is a Moodle plugin, if you've some experience of Moodle plugin programming, I'd be glad to look at your PR.

Be sure please to conform to the Moodle coding style.

martignoni added a commit to moodlebox/moodlebox that referenced this issue Nov 10, 2018

martignoni added a commit that referenced this issue Nov 11, 2018

Implement SSID hiding option
- Display of current hiding setting implemented.
- Moodle form updated.
- See issue #56.

martignoni added a commit that referenced this issue Nov 11, 2018

martignoni added a commit that referenced this issue Nov 11, 2018

Language strings added for SSID hiding feature
- In english and french.
- Issue #56.

martignoni added a commit to moodlebox/moodlebox that referenced this issue Nov 11, 2018

martignoni added a commit that referenced this issue Nov 11, 2018

@martignoni

This comment has been minimized.

Member

martignoni commented Nov 11, 2018

Implementation done. Needs testing now.

@adpe

This comment has been minimized.

adpe commented Nov 14, 2018

Thanks for working on it. I copied the two lines manually to the hostapd.conf. So this automated init part is not covered by my tests. Here is my review:

[y] Syntax (I'm not so experienced, please take it with attention)
[y] Output (Expected result on admin view)
[y] Language (en/fr)
[-] Databases
[y] Testing (Does the function do what it should => Moodle plugin and Shell script)
[-] Security
[-] Performance and Clustering
[n] Documentation
[y] Git
[-] Third party code
[-] Sanity check
[-] Icons

Just some questions:

  • How does the trigger work? I've noticed that the filepermissions and owner of changewifisettings.sh changed after saving new Wi-Fi settings. They're not anymore moodlebox:www-data, they're root:root.
  • Does the german translation will be made with AMOS? Why isn't just en in the plugin and fr will done with AMOS too? Is there a reason?
  • Would it not be better, if the triggerfiles will be ignored from git? Would make sense to add these to .gitignore.
@martignoni

This comment has been minimized.

Member

martignoni commented Nov 14, 2018

Thanks for this review 👍

To your questions.

How does the trigger work?

There's an incron job that checks if the hidden triggerfile is changed. In this case, the script changewifisettings.sh is launched and sets the adequate parameters in hostapd.conf.

Does the german translation will be made with AMOS? Why isn't just en in the plugin and fr will done with AMOS too? Is there a reason?

The german translation is indeed done via AMOS (and other languages too), like in any Moodle plugin. The english file must be in the source (as per Moodle coding way). I keep the french language file here for archive reasons (but the strings are also translated in AMOS).

Would it not be better, if the triggerfiles will be ignored from git?

It was the case some time ago, but this broke the upgrade process. See issue #37.

@martignoni

This comment has been minimized.

Member

martignoni commented Dec 1, 2018

Tested. Works OK.

martignoni added a commit that referenced this issue Dec 1, 2018

martignoni added a commit to moodlebox/moodlebox.net that referenced this issue Dec 1, 2018

@martignoni martignoni closed this Dec 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment