Skip to content

Generate a code for meetings registrations#3805

Merged
mrcasals merged 5 commits intomasterfrom
rbngzlv/meeting_registration_codes
Jul 10, 2018
Merged

Generate a code for meetings registrations#3805
mrcasals merged 5 commits intomasterfrom
rbngzlv/meeting_registration_codes

Conversation

@rbngzlv
Copy link
Copy Markdown
Contributor

@rbngzlv rbngzlv commented Jul 9, 2018 •
edited
Loading

🎩 What? Why?

Registered participants receive a registration code (8 digit alphanumeric only capital letters and excluding ambiguous characters such as 1, O/0, etc.)

📌 Related Issues

📋 Subtasks

  • Add CHANGELOG entry
  • Add tests

@rbngzlv rbngzlv self-assigned this Jul 9, 2018
@ghost ghost added the status: WIP label Jul 9, 2018
@rbngzlv rbngzlv force-pushed the rbngzlv/meeting_registration_codes branch from 53f1e4d to d58e308 Compare July 9, 2018 11:40
rbngzlv
rbngzlv commented Jul 9, 2018
View reviewed changes
decidim-meetings/spec/lib/meetings/registrations/code_generator_spec.rb
.to receive(:choose)
.with(length)
.exactly(2).times
.and_return(existing_code, valid_code)
Copy link
Copy Markdown
Contributor Author

@rbngzlv rbngzlv Jul 9, 2018

Choose a reason for hiding this comment

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

@mrcasals, @deivid-rodriguez I need help testing this. Rubocop complains with RSpec/SubjectStub: Do not stub your test subject, can you help me with another way to test this method?

Class generating the code: https://github.com/decidim/decidim/pull/3805/files#diff-6c225d57a5e2477b37660748966d7262R18

Copy link
Copy Markdown
Contributor

@deivid-rodriguez deivid-rodriguez Jul 9, 2018

Choose a reason for hiding this comment

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

Test looks good and readable to me, I would disable that cop for the test.

Copy link
Copy Markdown
Contributor

@mrcasals mrcasals Jul 9, 2018

Choose a reason for hiding this comment

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

👍 the only thing that comes to mind is moving the choose method to a different object so you can stub it, but that'd be overengineering it IMO.

Copy link
Copy Markdown
Contributor Author

@rbngzlv rbngzlv Jul 9, 2018

Choose a reason for hiding this comment

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

Thanks to both, I agree with your comments, so I'll disable the cop for the test.

@xabier xabier mentioned this pull request Jul 9, 2018
Closed
6 tasks
@rbngzlv
Copy link
Copy Markdown
Contributor Author

rbngzlv commented Jul 10, 2018

@decidim/lot-core ready to review!

mrcasals
mrcasals approved these changes Jul 10, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@mrcasals mrcasals left a comment

Choose a reason for hiding this comment

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

Wohoo, nice job!

@mrcasals mrcasals merged commit c32bd92 into master Jul 10, 2018
@mrcasals mrcasals deleted the rbngzlv/meeting_registration_codes branch July 10, 2018 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@deivid-rodriguez deivid-rodriguez deivid-rodriguez left review comments

@mrcasals mrcasals mrcasals approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@rbngzlv rbngzlv

Labels

in-review module: meetings

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rbngzlv @mrcasals @deivid-rodriguez