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

Use internal census system #71

Merged
merged 171 commits into from
Mar 11, 2024
Merged

Conversation

antopalidi
Copy link
Collaborator

@antopalidi antopalidi commented Nov 2, 2023

fixes #60

  • add internal and external types for census
    Screenshot 2024-01-24 at 13 21 27

  • If the user is not logged in, a modal window appears when trying to start voting:
    Screenshot 2023-11-22 at 09 04 27

  • If the election is set up with an internal census and any type of verification is selected, a modal window with required missing verifications will be displayed when the user tries to start voting:
    Screenshot 2023-11-22 at 09 05 10

  • add "Update census" button to steps page, add info about the update

Screenshot 2024-02-07 at 14 00 18

Copy link

codecov bot commented Nov 2, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (node-backend@65c331c). Click here to learn what that means.

❗ Current head ed05175 differs from pull request most recent head 719da20. Consider uploading reports for the commit 719da20 to get more accurate results

Additional details and impacted files
@@               Coverage Diff               @@
##             node-backend      #71   +/-   ##
===============================================
  Coverage                ?   92.90%           
===============================================
  Files                   ?       94           
  Lines                   ?     1902           
  Branches                ?        0           
===============================================
  Hits                    ?     1767           
  Misses                  ?      135           
  Partials                ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@microstudi microstudi left a comment

Choose a reason for hiding this comment

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

Initial comments!

Gemfile Outdated Show resolved Hide resolved
app/models/decidim/vocdoni/voter.rb Show resolved Hide resolved
lib/tasks/schedule.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@microstudi microstudi left a comment

Choose a reason for hiding this comment

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

Nice job, we are close to finish it.

I would like to suggest in this review some changes.
On one side, I would prefer to use more semantic name ins general (lets call internal_census instead of permissions for instance). I've left some examples in the comments.

I'd also improve a little the admin page, here is my suggestion:

image

Finally, I give you a suggestion to properly obtain valid users (from the organization, not deleted) with or without authorizations depending on the configurations.

app/models/decidim/vocdoni/election.rb Outdated Show resolved Hide resolved
app/models/decidim/vocdoni/voter.rb Outdated Show resolved Hide resolved
app/forms/decidim/vocdoni/admin/census_permissions_form.rb Outdated Show resolved Hide resolved
app/forms/decidim/vocdoni/admin/census_permissions_form.rb Outdated Show resolved Hide resolved
app/forms/decidim/vocdoni/admin/census_permissions_form.rb Outdated Show resolved Hide resolved
@antopalidi antopalidi mentioned this pull request Feb 23, 2024
Copy link
Collaborator

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

I have some suggestions for improving the feature. Can you check them out 🙏🏽 ?

.github/workflows/test.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
app/models/decidim/vocdoni/election.rb Outdated Show resolved Hide resolved
app/packs/src/decidim/vocdoni/admin/steps/update_census.js Outdated Show resolved Hide resolved
app/packs/src/decidim/vocdoni/admin/steps/update_census.js Outdated Show resolved Hide resolved
node-wrapper/test_census.mjs Outdated Show resolved Hide resolved
@antopalidi antopalidi requested a review from andreslucena March 4, 2024 17:40
@antopalidi
Copy link
Collaborator Author

@andreslucena Andres, all the changes have been made

Copy link
Collaborator

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

I have more feedback, can you check it out?

spec/system/decidim/vote_online_spec.rb Outdated Show resolved Hide resolved
spec/services/decidim/vocdoni/voter_service_spec.rb Outdated Show resolved Hide resolved
spec/services/decidim/vocdoni/voter_service_spec.rb Outdated Show resolved Hide resolved
config/locales/en.yml Outdated Show resolved Hide resolved
app/controllers/decidim/vocdoni/admin/steps_controller.rb Outdated Show resolved Hide resolved
app/controllers/decidim/vocdoni/admin/steps_controller.rb Outdated Show resolved Hide resolved
app/controllers/decidim/vocdoni/admin/census_controller.rb Outdated Show resolved Hide resolved
app/controllers/decidim/vocdoni/admin/census_controller.rb Outdated Show resolved Hide resolved
app/controllers/decidim/vocdoni/admin/census_controller.rb Outdated Show resolved Hide resolved
@antopalidi antopalidi requested a review from andreslucena March 6, 2024 12:15
Copy link
Collaborator

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

About the data selectors, I'm seeing that we have lots of them, so maybe we can move the task to an issue to not add too much workload on this PR.

app/controllers/decidim/vocdoni/admin/census_controller.rb Outdated Show resolved Hide resolved
node-wrapper/test_census.mjs Outdated Show resolved Hide resolved
spec/system/decidim/admin/admin_manages_census_spec.rb Outdated Show resolved Hide resolved
spec/system/decidim/admin/admin_manages_census_spec.rb Outdated Show resolved Hide resolved
spec/system/decidim/admin/admin_manages_census_spec.rb Outdated Show resolved Hide resolved
app/packs/src/decidim/vocdoni/admin/census.js Outdated Show resolved Hide resolved
app/packs/src/decidim/vocdoni/admin/census.js Outdated Show resolved Hide resolved
app/packs/src/decidim/vocdoni/admin/steps/update_census.js Outdated Show resolved Hide resolved
app/packs/src/decidim/vocdoni/admin/steps/update_census.js Outdated Show resolved Hide resolved
app/packs/src/decidim/vocdoni/admin/steps/update_census.js Outdated Show resolved Hide resolved
@antopalidi antopalidi requested a review from andreslucena March 7, 2024 11:36
@antopalidi
Copy link
Collaborator Author

@andreslucena all done

Copy link
Collaborator

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

Approving this PR as I'm happy with the result.

Thanks for your patience @antopalidi !! This looks great 👏🏽 👏🏽

@andreslucena andreslucena merged commit 1b907ea into main Mar 11, 2024
2 checks passed
@microstudi microstudi deleted the feature/internal-census-system branch May 22, 2024 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use the internal census system
3 participants