Causes an integrity constraint violation. Should be an error, but error should occur more gracefully.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

AohRveTPV’s picture

Issue summary: View changes
NancyDru’s picture

Status: Active » Needs review
FileSize
1014 bytes

Will this do?

Status: Needs review » Needs work

The last submitted patch, 2: 2498595.patch, failed testing.

NancyDru’s picture

I don't really understand tests.

AohRveTPV’s picture

Testbot could not apply your patch. It looks to me like the problem is the patch file does not have a newline character at the end of the last line in the file. I'm not sure how that happens. Vim indicated this with "[noeol]". Resaving seemed to fix.

By the way, it looks like line 17 of the patch adds a trailing space.

Thanks for the patch.

AohRveTPV’s picture

Status: Needs work » Needs review
FileSize
1012 bytes

Fixed aforementioned newline problem and removed trailing space.

Status: Needs review » Needs work

The last submitted patch, 6: 2498595-6.patch, failed testing.

NancyDru’s picture

i don't see why that patch would fail on expiration.

AohRveTPV’s picture

Running tests locally you can get a snapshot of the page after each request. That should be elucidating. Am running the tests locally now.

AohRveTPV’s picture

The problem with the code in the patch: If you edit and save an existing policy, you get "Duplicate policy name."

To reproduce: Create a new policy, save, then edit, and re-save.

The test failed because it just happens to edit an existing policy as part of the test, which causes the "Duplicate policy name."

NancyDru’s picture

Okay, back to the drawing board...

NancyDru’s picture

Status: Needs work » Needs review
FileSize
1.07 KB

Try it again.

Status: Needs review » Needs work

The last submitted patch, 12: 2498595-12.patch, failed testing.

NancyDru’s picture

Status: Needs work » Needs review
FileSize
1.07 KB

aarrgghh

Status: Needs review » Needs work

The last submitted patch, 14: 2498595-14.patch, failed testing.

NancyDru’s picture

I give up.

AohRveTPV’s picture

Same problem as in #2: no newline character at end of last line in patch file. Are you producing your patches using Git? The following should work:

git diff >foo.patch

NancyDru’s picture

Yes, that's what I'm doing, but there are other patches applied, so they are all stuck together and I have to try to pull them apart.

NancyDru’s picture

Status: Needs work » Needs review
FileSize
1.07 KB

I pulled a fresh version.

AohRveTPV’s picture

Are you familiar with git add -p? That is what I use most often for picking out some changes from many for a patch. You add selectively to the index from the working tree, then git diff --staged >foo.patch to diff the index versus HEAD. Maybe you already know about it, but I had been using Git for a while before I started using it.

NancyDru’s picture

Nope, never ran into that one.

AohRveTPV’s picture

FileSize
1.07 KB

Looks good to me. Two minor changes:
1. Remove hyphen from "case-sensitive". (I think the grammar rule is compound modifiers following the noun they modify do not get a hyphen.)
2. Change $dup_name to $dupe_name to avoid abbreviation. ("dupe" is informal adjective meaning the same as duplicate per at least one dictionary.)

It might be more conventional to solve this by having a human-readable name and a machine name--not sure. But this patch seems to fix the bug and that is most important.

The return TRUE; line in the validate function is pointless, I think, but it can be removed separately.

  • AohRveTPV committed a451185 on 7.x-2.x authored by NancyDru
    Issue #2498595 by NancyDru, AohRveTPV: Adding two policies with same...
AohRveTPV’s picture

Status: Needs review » Fixed

Went ahead and committed. Thanks for the fix!

The changes in #22 are not functional changes and can be fixed later if I have erred.

NancyDru’s picture

Thanks. Sorry I didn't get to it over the weekend.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.