feat:added toggle methods to basemodel #293

Theo-flux · 2023-07-08T01:29:00Z

Description:
Enhance BaseToggle with additional methods.
Add the following methods to enhance BaseToggle abstract class
is_disabled
is_toggled_on
is_toggled_off

discussion forum: https://discuss.openedx.org/t/ideas-for-naming-methods-in-a-class/10630
Github issue: #290

Dependencies: dependencies on other outstanding PRs, issues, etc.

Merge deadline: List merge deadline (if any)

Installation instructions: List any non-trivial installation
instructions.

Testing instructions:

Open page A
Do thing B
Expect C to happen
If D happened instead - check failed.

Reviewers:

@robrap

Merge checklist:

Post merge:

Create a tag
Check new version is pushed to PyPi after tag-triggered build is
finished.
Delete working branch (if not needed anymore)

Author concerns: List any concerns about this PR - inelegant
solutions, hacks, quick-and-dirty implementations, concerns about
migrations, etc.

openedx-webhooks · 2023-07-08T01:29:04Z

Thanks for the pull request, @Theo-flux! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

robrap · 2023-07-10T19:14:31Z

@mphilbrick211: You can ping me once Contributor Agreement is complete. Thank you!

e0d · 2023-07-10T19:57:29Z

@Theo-flux please see the details about our CLA requirement in this comment.

Theo-flux · 2023-07-10T20:33:30Z

@Theo-flux please see the details about our CLA requirement in this comment.

@ I have gone through it. Kindly let me know what exactly I am to do if there is one. Thanks

e0d · 2023-07-10T20:54:40Z

Did you receive a document for e-signature yet?

robrap · 2023-07-12T20:10:34Z

@Theo-flux: Ed had written:

Did you receive a document for e-signature yet?

Can you respond. You still don't have a signed Contributor Agreement and I can't proceed with the review until that is complete. Thank you.

Theo-flux · 2023-07-13T07:18:31Z

@Theo-flux: Ed had written:

Did you receive a document for e-signature yet?

Can you respond. You still don't have a signed Contributor Agreement and I can't proceed with the review until that is complete. Thank you.
@robrap I am still yet to receive a mail from docuSign. Is there anything that can be done to speed this up ?

robrap · 2023-07-13T11:21:39Z

@Theo-flux: I’m not sure if your response was lost, but the only thing you included was a quote.

Theo-flux · 2023-07-13T11:26:30Z

@robrap I haven't received any mail from docusign for the e-signature. However, I send another request for the CLA. Let's see how it goes before the end of tomorrow. Sorry about the delay. Thanks.

robrap · 2023-07-13T11:31:53Z

@Theo-flux: Thanks and good luck.

Also, this time you commented twice, so not sure if you are experiencing connectivity issues.

Theo-flux · 2023-07-13T11:36:20Z

OK. Yes I have connectivity issues at the moment. Thanks.

Theo-flux · 2023-07-17T14:26:32Z

@robrap I am still yet to receive the mail. Could this be a problem from my end ?

mphilbrick211 · 2023-07-17T18:38:28Z

Hi @Theo-flux! I checked on the CLA for you, and it shows we sent it to you on July 10th, but that it hasn't been completed. Our legal counsel is out-of-office, so she may not have signed yet and that's why it's showing as incomplete. If you've already signed, we can have someone sign in her absence. Thanks!

Theo-flux · 2023-07-17T19:47:33Z

Hi @Theo-flux! I checked on the CLA for you, and it shows we sent it to you on July 10th, but that it hasn't been completed. Our legal counsel is out-of-office, so she may not have signed yet and that's why it's showing as incomplete. If you've already signed, we can have someone sign in her absence. Thanks!

Hello @mphilbrick211 I just searched my mails again between 10 July till now and i found nothing. Kindly let me know what next to do. Thanks.

mphilbrick211 · 2023-07-17T22:56:00Z

Hi @Theo-flux! I checked on the CLA for you, and it shows we sent it to you on July 10th, but that it hasn't been completed. Our legal counsel is out-of-office, so she may not have signed yet and that's why it's showing as incomplete. If you've already signed, we can have someone sign in her absence. Thanks!

Hello @mphilbrick211 I just searched my mails again between 10 July till now and i found nothing. Kindly let me know what next to do. Thanks.

Thanks for confirming, @Theo-flux - I'm looking into this for you!

Theo-flux · 2023-07-21T10:16:35Z

@robrap and @mphilbrick211 Good day. I hope the day is going well ? I would like to know if there is any update concerning the CLA agreement. Thanks for your responses so far.

e0d · 2023-07-21T12:53:47Z

@Theo-flux you should have received a message from DocuSign on or around the 13th. Can you double check?

Theo-flux · 2023-07-21T13:05:06Z

@e0d I have gone through my mail still, I have nothing relating to DocuSign. Is it advisable I send another CLA request now ?

e0d · 2023-07-21T16:00:55Z

@Theo-flux can send an email to [email protected]? I suppose there's some chance there was a typo in your email address or similar. We can work through that in a private channel.

Theo-flux · 2023-07-21T16:54:51Z

@e0d Thanks, I just sent the mail now.

Theo-flux · 2023-07-21T20:33:53Z

@mphilbrick211 @robrap @e0d Thanks for your help. The CLA has been signed and sent back. Thanks alot!

Theo-flux · 2023-07-22T06:08:29Z

I think i just mistakenly closed a comment or so @robrap

robrap · 2023-08-16T21:38:32Z

@Theo-flux: I'm curious what your plans are for this. Thank you.

Theo-flux · 2023-08-17T06:22:12Z

@Theo-flux: I'm curious what your plans are for this. Thank you.

@robrap I am sorry for the delay. I have written the test case for the methods in the BaseWaffleTest class.

robrap

This is great. Thank you @Theo-flux.

I added some comments on the tests, but it should be very quick work.
Let's hold off on updating docs. I have the following doc PR open that would cause conflicts, and I can just update that PR when this lands. See Fix and improve toggle docs #303
Please update the changelog with a 5.2.0 release for this change, and update the version from 5.1.0 to 5.2.0.

Thanks!

robrap · 2023-08-17T15:34:14Z

edx_toggles/tests/test_waffle.py

+        #test is_disabled method
+        self.assertEqual(False, waffle.is_disabled())
+        #test is_toggle_on method
+        self.assertEqual(waffle.is_enabled(), waffle.is_toggle_on())


Nit: I think it would be quicker to read if you just stuck with True/False for the final two assertions as well.

@robrap I was a bit confused about the third statement on updating the changelog. Just to be sure now, I only need to change from [5.1.0] to [5.2.0] right ?

You'll want to add a new section to the top of the changelog for version 5.2.0. It should be a similar format as the sections below, like this example for 5.1.0, but you'll update it to read "5.2.0", and update the date, and it will have one bullet like:

* Added is_disabled, is_toggle_on, and is_toggle_off methods to BaseToggle.

[request] As I was writing the above, it made me think that we should probably rename is_toggle_on/off to is_toggled_on/off to be more consistent with the past tense of enabled/disabled? What do you think?

You'll need to change the actual version to 5.2.0 in init.py as well. It looks like someone updated it to 5.1.1, but that doesn't affect this.

Thanks for these updates. Marking resolved.

Actually, unmarking as resolved, because failing tests indicate that the is_toggle_on rename is not complete. Sorry about that.

edx_toggles/tests/test_waffle.py

mphilbrick211 · 2023-08-17T19:28:46Z

Hi @robrap! Would you mind enabling the tests on this again? Thanks!

robrap

Getting close. Thank you.

edx_toggles/toggles/internal/base.py

robrap · 2023-10-16T19:11:27Z

@Theo-flux:

Tests are failing again. It should be a quick fix, as noted in feat:added toggle methods to basemodel #293 (comment), and it would be great to land this sooner rather than later.
I updated the PR description with a link to the ticket Enhance BaseToggle with additional enabled/disabled toggle methods #290, and updated the ticket with the additional TODO items discussed in this PR. Once you land this, we can see what your appetite is for completing the work, or if we should open a new ticket with just the remaining work.

Theo-flux · 2023-10-17T03:09:48Z

@robrap on it.

robrap · 2023-10-17T14:06:03Z

edx_toggles/tests/test_waffle.py

+        self.assertEqual(True, waffle.is_enabled())
+        # test is_disabled method
+        self.assertEqual(False, waffle.is_disabled())
+        # test is_toggle_on method


@Theo-flux: This comment is out of sync. Rather than updating it, I think the assertions are clear enough as-is and you could simply delete all of these # test ... comments from these two tests.

@Theo-flux? 😄

@Theo-flux: I'd love to have you be able to land this. Checking in one more time. Otherwise - I may end up taking this over at some point. Just let me know. Thanks.

mphilbrick211 · 2023-11-15T18:01:32Z

HI @Theo-flux! Just checking in on this!

robrap · 2023-11-15T21:13:33Z

Hello @Theo-flux. I really appreciate the changes, and we'd like to help land this if you are too busy with other things. Please let us know if you'd like to complete the work, or if you are ok with us taking it over from here. If we don't hear back in the next 2-3 weeks, we will take it over with lots of gratitude for your work thus far. Thank you!

robrap · 2023-12-05T17:00:00Z

Hello again @Theo-flux. Again, I really appreciate the changes. I linked to your PR from the original ticket, and someone will take that up and land it at some point. In the meantime, I am going to close this PR. Thank you again and good luck with whatever you are working on now.

openedx-webhooks · 2023-12-05T17:00:05Z

@Theo-flux Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

feat: added toggle methods to basemodel

8014609

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jul 8, 2023

mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jul 10, 2023

e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jul 10, 2023

Theo-flux changed the title ~~feat: added toggle methods to basemodel~~ feat:added toggle methods to basemodel Jul 10, 2023

e0d changed the title ~~feat:added toggle methods to basemodel~~ feat:added toggle methods to basemodel. Jul 12, 2023

e0d changed the title ~~feat:added toggle methods to basemodel.~~ feat:added toggle methods to basemodel Jul 12, 2023

Merge branch 'master' into theo-flux/toggles-add-methods-290

a48dc14

e0d changed the title ~~feat:added toggle methods to basemodel~~ feat:added toggle methods to basemodel Jul 21, 2023

Theo-flux closed this Jul 22, 2023

robrap reopened this Jul 23, 2023

e0d and others added 2 commits August 16, 2023 18:38

Merge branch 'master' into theo-flux/toggles-add-methods-290

ce556a8

test:toggle methods' testcase in BaseWaffleTest class

c370991

robrap reviewed Aug 17, 2023

View reviewed changes

robrap mentioned this pull request Aug 17, 2023

Fix and improve toggle docs #303

Merged

4 tasks

test: added test_toggle_methods_with_disabled_waffle in BaseWaffleTest

9f0b6ae

robrap reviewed Aug 18, 2023

View reviewed changes

edx_toggles/toggles/internal/base.py Outdated Show resolved Hide resolved

refactor: is_disabed changed to is_disabled

08ce0d9

timmc-edx assigned robrap Sep 25, 2023

robrap assigned Theo-flux and unassigned robrap Sep 26, 2023

robrap added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Sep 29, 2023

refactor: removed whitespace from file.

60ee15c

robrap mentioned this pull request Oct 16, 2023

Enhance BaseToggle with additional enabled/disabled toggle methods #290

Open

3 tasks

Merge branch 'master' into theo-flux/toggles-add-methods-290

85dbd91

fix: is_toggle_on/of changed to is_toggled_on/of

e636210

fix: is_toggle_on/of changed to is_toggled_on/of

4d4c17e

robrap reviewed Oct 17, 2023

View reviewed changes

robrap closed this Dec 5, 2023

feat:added toggle methods to basemodel #293

feat:added toggle methods to basemodel #293

Conversation

Theo-flux commented Jul 8, 2023 • edited by robrap Loading

openedx-webhooks commented Jul 8, 2023 • edited Loading

robrap commented Jul 10, 2023

e0d commented Jul 10, 2023

Theo-flux commented Jul 10, 2023

e0d commented Jul 10, 2023

robrap commented Jul 12, 2023

Theo-flux commented Jul 13, 2023

robrap commented Jul 13, 2023

Theo-flux commented Jul 13, 2023

robrap commented Jul 13, 2023

Theo-flux commented Jul 13, 2023

Theo-flux commented Jul 17, 2023

mphilbrick211 commented Jul 17, 2023

Theo-flux commented Jul 17, 2023

mphilbrick211 commented Jul 17, 2023

Theo-flux commented Jul 21, 2023 • edited Loading

e0d commented Jul 21, 2023

Theo-flux commented Jul 21, 2023

e0d commented Jul 21, 2023

Theo-flux commented Jul 21, 2023

Theo-flux commented Jul 21, 2023

Theo-flux commented Jul 22, 2023

robrap commented Aug 16, 2023

Theo-flux commented Aug 17, 2023

robrap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mphilbrick211 commented Aug 17, 2023

robrap left a comment

Choose a reason for hiding this comment

robrap commented Oct 16, 2023

Theo-flux commented Oct 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mphilbrick211 commented Nov 15, 2023

robrap commented Nov 15, 2023

robrap commented Dec 5, 2023

openedx-webhooks commented Dec 5, 2023

Theo-flux commented Jul 8, 2023 •

edited by robrap

Loading

openedx-webhooks commented Jul 8, 2023 •

edited

Loading

Theo-flux commented Jul 21, 2023 •

edited

Loading