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

[change] Metric collection: added consent in web UI #372 #378

Merged
merged 5 commits into from
Apr 23, 2024

Conversation

pandafy
Copy link
Member

@pandafy pandafy commented Apr 15, 2024

Closes #372

Changes

#Please explain changes made in your PR#

Screenshots

#For UI changes, please share a screenshot#

Checklist

  • I have read the contributing guidelines.
  • I have manually tested the proposed changes.
  • I have written new test cases to avoid regressions. (if necessary)
  • I have updated the documentation. (e.g. README.rst)
  • I have added [change!] to commit title to indicate a backward incompatible change. (if required)
  • I have checked the links added / modified in the documentation.

#Closes #(issue-number)#

@coveralls
Copy link

coveralls commented Apr 15, 2024

Coverage Status

coverage: 98.403% (+0.003%) from 98.4%
when pulling a10ce63 on issues/372-metric-collection-web-consent
into 95c8e9a on master.

@pandafy pandafy force-pushed the issues/372-metric-collection-web-consent branch 4 times, most recently from a6f8f23 to fee1f50 Compare April 15, 2024 16:25
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.

See my changes to the README here: 95c8e9a, please mention the possibility of removing consent from the UI there too.

openwisp_utils/measurements/models.py Outdated Show resolved Hide resolved
<h2>Metric collection</h2>
<form method="POST" id="id_metric_collection_consent_form">
{% csrf_token %}
{{ metric_consent_form }}
Copy link
Member

Choose a reason for hiding this comment

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

Please rewrite this so that we can have the input first and each line wrapped on a <p> element, the current output is not readable.

You probably need to add:

    margin-right: 8px;
    vertical-align: top;

To the checkbox input to make it look decent:

Screenshot from 2024-04-16 11-52-42

Copy link
Member Author

Choose a reason for hiding this comment

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

I just used some CSS to achieve the same result. Re-writing this would require copying some code from Django which we will need to maintain over time.

@pandafy pandafy force-pushed the issues/372-metric-collection-web-consent branch from cd6c684 to 7cb4720 Compare April 17, 2024 03:00
@pandafy pandafy force-pushed the issues/372-metric-collection-web-consent branch from 7cb4720 to 5799c8c Compare April 17, 2024 03:05
pandafy and others added 3 commits April 18, 2024 21:49
- renamed metrics_collection to metric_collection
- reduced verbosity
- isolated code meant to be used in admin_theme.admin
- avoided used of term "disclaimer"
- moved most of the code in class methods of models.OpenwispVersion
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.

Looks pretty good to me now after these last iterations.
I am merging, if you spot anything else that needs improvement please send a new PR.

@nemesifier nemesifier merged commit 1d3b00c into master Apr 23, 2024
20 checks passed
@nemesifier nemesifier deleted the issues/372-metric-collection-web-consent branch April 23, 2024 00:12
nemesifier added a commit to openwisp/ansible-openwisp2 that referenced this pull request Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

[change] Metric collection: add consent in web UI
3 participants