Edgewall Software
Modify

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#11677 closed defect (fixed)

Deleting default Component, Milestone, Version or Enum doesn't clear default value

Reported by: Ryan J Ollos Owned by: walty <walty8@…>
Priority: low Milestone: 1.2
Component: ticket system Version: 1.0.1
Severity: normal Keywords: component, milestone, version, enum
Cc: walty8@… Branch:
Release Notes:

Deleting the default Component, Milestone, Version or Enum clears the complementary default value (e.g. [ticket] default_component).

API Changes:

Description

Deleting the default Component, Milestone, Version or Enum leaves the [ticket] default_ entry in trac.ini having a non-existent resource id for a value. The entry could be cleared when the resource is deleted. For example, if [ticket] default_milestone = milestone1 and milestone1 is deleted, the entry could be cleared, [ticket] default_milestone = .

Attachments (3)

11677-clear-default.v1.patch (3.6 KB ) - added by walty <walty8@…> 4 years ago.
11677-clear-default.v2.patch (5.6 KB ) - added by walty <walty8@…> 4 years ago.
11677-clear-default.v3.patch (11.0 KB ) - added by walty <walty8@…> 4 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 by Ryan J Ollos, 5 years ago

Milestone: next-stable-1.0.xnext-major-releases

comment:2 by Ryan J Ollos, 5 years ago

Priority: normallow

comment:3 by walty <walty8@…>, 4 years ago

Cc: walty8@… added

comment:4 by walty <walty8@…>, 4 years ago

I suppose Priority, Resolution, Severity, Ticket Type and Version would do the same thing?

btw, I am not sure where is the Enum..?

in reply to:  4 comment:5 by walty <walty8@…>, 4 years ago

Replying to walty <walty8@…>:

I suppose Priority, Resolution, Severity, Ticket Type and Version would do the same thing?

btw, I am not sure where is the Enum..?

pls ignore this comment, I just found the inheritance between the four and Enum class in the code.

by walty <walty8@…>, 4 years ago

comment:6 by walty <walty8@…>, 4 years ago

A patch was just uploaded, please advise on the best practice/sample of the unit tests of this one.

btw, I found a minor bug during testing the patch, clearing default of Enum values (e.g. priority) would crash, I have fixed that as well (at the end of the patch). I am not sure if I should exclude that part from the patch.

thanks.

in reply to:  6 ; comment:7 by Ryan J Ollos, 4 years ago

Replying to walty <walty8@…>:

btw, I found a minor bug during testing the patch, clearing default of Enum values (e.g. priority) would crash, I have fixed that as well (at the end of the patch). I am not sure if I should exclude that part from the patch.

Thanks for catching that. Fixed in [14267] (see comment:11:ticket:11765).

in reply to:  7 ; comment:8 by walty <walty8@…>, 4 years ago

Replying to rjollos:

Replying to walty <walty8@…>:

btw, I found a minor bug during testing the patch, clearing default of Enum values (e.g. priority) would crash, I have fixed that as well (at the end of the patch). I am not sure if I should exclude that part from the patch.

Thanks for catching that. Fixed in [14267] (see comment:11:ticket:11765).

You are welcome, I think I should remove that part from the patch then.

btw, I guess I better add the functional tests at the same place (tests/functional/main.py) for the new code?

in reply to:  8 comment:9 by Ryan J Ollos, 4 years ago

Replying to walty <walty8@…>:

btw, I guess I better add the functional tests at the same place (tests/functional/main.py) for the new code?

You could write unit tests since only the logic in admin.py needs to be tested. The functional tests are slow so we tend to only use them when template code needs to be tested. I added an admin test module in [14271:14272].

by walty <walty8@…>, 4 years ago

comment:10 by walty <walty8@…>, 4 years ago

hi, the new patch includes the unit tests for the components (clean up default value of trac.ini), pls advise if this is fine. I would work on other modules if this patch is OK.

two further questions here:

  1. I found that the unit test file of admin is very simple now, so for my patch, do I need to write the unit tests for my patch only, or I better write the unit tests of others as well (e.g. add/remove priorities).
  1. Since the unit tests of these admin panels are very alike, do u mind if I use some sort of inheritance inside the test suite so the code could be more effectively written? or it's actually a bad practice in the field of unit test?

thanks.

in reply to:  10 comment:11 by Ryan J Ollos, 4 years ago

Replying to walty <walty8@…>:

hi, the new patch includes the unit tests for the components (clean up default value of trac.ini), pls advise if this is fine.

Ideally each test case will test just one thing. component1 and component2 are added when the EnvironmentStub is created with default_data=True so you can work with those components to avoid needing to add a component to setup your test case.

  1. I found that the unit test file of admin is very simple now, so for my patch, do I need to write the unit tests for my patch only, or I better write the unit tests of others as well (e.g. add/remove priorities).

I think you can just focus on adding coverage for your patch, unless you potential for breaking some other behavior and want to add coverage just to be sure.

  1. Since the unit tests of these admin panels are very alike, do u mind if I use some sort of inheritance inside the test suite so the code could be more effectively written? or it's actually a bad practice in the field of unit test?

I think it's fine to use inheritance, especially when creating a mixin class. You could find many examples in our test suite in which we've used a base class to avoid test duplication.

by walty <walty8@…>, 4 years ago

comment:12 by walty <walty8@…>, 4 years ago

hi,

I have uploaded a new patch, with base class and mixin class added into the unit test. My main reference is tracopt/versioncontrol/git/tests/git_fs.py. Please see if this version is fine.

thanks

comment:13 by Ryan J Ollos, 4 years ago

Milestone: next-major-releases1.2
Owner: set to Ryan J Ollos
Status: newassigned

comment:14 by Ryan J Ollos, 4 years ago

I reviewed the patch and will push revised changes soon. No need to update the patch, but a few comments for future consideration:

  • Consider reviewing PEP-0008 and using a linter on your changes.
  • Ideally tests should be a bit more isolated from one another, each testing only a single behavior.
  • There were a few copy/paste errors in the test that didn't lead to actual failures, but made the tests confusing to read.

Those are just small things and the changes look good.

in reply to:  14 comment:15 by walty <walty8@…>, 4 years ago

Replying to Ryan J Ollos:

I reviewed the patch and will push revised changes soon. No need to update the patch, but a few comments for future consideration:

  • Consider reviewing PEP-0008 and using a linter on your changes.
  • Ideally tests should be a bit more isolated from one another, each testing only a single behavior.
  • There were a few copy/paste errors in the test that didn't lead to actual failures, but made the tests confusing to read.

Those are just small things and the changes look good.

thanks a lot for your review and comments!

comment:16 by Ryan J Ollos, 4 years ago

Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Committed to trunk in [14292].

comment:17 by Ryan J Ollos, 4 years ago

Owner: changed from Ryan J Ollos to walty <walty8@…>

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain walty <walty8@…>.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from walty <walty8@…> to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.