a fix for duplicate alias problem

[godplayer56]

This pull request tries to fix the duplicate-alias issue #2942

[godplayer56]

@bhearsum @gabrielBusta I am seeing that one of the tests failed for the PR 😵‍💫.

I had used the same function which has been used for rules. How should I change the function for the test to pass?

[bhearsum]

@bhearsum @gabrielBusta I am seeing that one of the tests failed for the PR 😵‍💫.

I had used the same function which has been used for rules. How should I change the function for the test to pass?

I can see that the failure is a bit confusing -- this is actually a failure in the Python file formatting - not in any test.

[task 2023-10-12T21:23:33.129Z] ERROR: InvocationError for command /builds/worker/checkouts/vcs/.tox/check/bin/black --diff --check scripts src taskcluster tests uwsgi (exited with code 1)

In addition to fixing that, it would be good to add tests for this in https://github.com/mozilla-releng/balrog/blob/main/tests/admin/views/test_rules.py (we already have a number of tests for these endpoints that should be a good model for new ones).

[godplayer56]

@bhearsum @gabrielBusta I am seeing that one of the tests failed for the PR 😵‍💫. I had used the same function which has been used for rules. How should I change the function for the test to pass?

I can see that the failure is a bit confusing -- this is actually a failure in the Python file formatting - not in any test.

[task 2023-10-12T21:23:33.129Z] ERROR: InvocationError for command /builds/worker/checkouts/vcs/.tox/check/bin/black --diff --check scripts src taskcluster tests uwsgi (exited with code 1)

In addition to fixing that, it would be good to add tests for this in https://github.com/mozilla-releng/balrog/blob/main/tests/admin/views/test_rules.py (we already have a number of tests for these endpoints that should be a good model for new ones).

@bhearsum How should I approach fixing the python formatting issue? How should I check if the files have formatting issues?

I am thinking of adding a test similar to the test for checking alias in rules class

[bhearsum]

@bhearsum @gabrielBusta I am seeing that one of the tests failed for the PR 😵‍💫. I had used the same function which has been used for rules. How should I change the function for the test to pass?

I can see that the failure is a bit confusing -- this is actually a failure in the Python file formatting - not in any test.

[task 2023-10-12T21:23:33.129Z] ERROR: InvocationError for command /builds/worker/checkouts/vcs/.tox/check/bin/black --diff --check scripts src taskcluster tests uwsgi (exited with code 1)

In addition to fixing that, it would be good to add tests for this in https://github.com/mozilla-releng/balrog/blob/main/tests/admin/views/test_rules.py (we already have a number of tests for these endpoints that should be a good model for new ones).

@bhearsum How should I approach fixing the python formatting issue? How should I check if the files have formatting issues?

I am thinking of adding a test similar to the test for checking alias in rules class

You can use black to format the Python files correctly.

[godplayer56]

I have formatted the code but some tests still fail

[bhearsum]

I have formatted the code but some tests still fail

Please have a look through the linked log for any errors that are still showing up.

[godplayer56]

I have formatted the code but some tests still fail

Please have a look through the linked log for any errors that are still showing up.

I checked for errors but only warnings are being shown. I have formatted all the files using black, so it can be some other issue which I am not able to figure out from the logs :cry:

[gbrownmozilla]

Sorry, this shouldn't be so hard!

The log shown by github is incomplete; you can see the full test log if you "View logs in Taskcluster". In the full log, I see

[task 2023-10-16T10:43:40.522Z] ERROR: InvocationError for command /builds/worker/checkouts/vcs/.tox/check/bin/check-manifest -v . (exited with code 1)

...which doesn't tell you much! I wonder if that's related to the new file you are adding? I would suggest not including settings.json in the PR.

[godplayer56]

Sorry, this shouldn't be so hard!

The log shown by github is incomplete; you can see the full test log if you "View logs in Taskcluster". In the full log, I see

[task 2023-10-16T10:43:40.522Z] ERROR: InvocationError for command /builds/worker/checkouts/vcs/.tox/check/bin/check-manifest -v . (exited with code 1)

...which doesn't tell you much! I wonder if that's related to the new file you are adding? I would suggest not including settings.json in the PR.

Oh I checked the taskcluster logs but I wasn't able to see them :disappointed: , I'll be careful from now on! Here is what I see in the taskcluster logs even after logging in- For the settings.json file, I will remove it from the PR.

[godplayer56]

Hii @bhearsum @gbrownmozilla , the checks have passed, can you please review the PR?

Should I do some changes to it?

[godplayer56]

@bhearsum @gbrownmozilla I have added the tests for the code. Can you please have a look and tell me if it needs some changes?

[bhearsum]

@bhearsum @gbrownmozilla I have added the tests for the code. Can you please have a look and tell me if it needs some changes?

It looks like you're making progress. The tests that you added looks fine to me, but you need to cover some additional cases still:

• A POST to /scheduled_changes/rules with update_type set to update with a duplicate alias
• A POST to /scheduled_changes/rules/:sc_id with update_type set to insert with a duplicate alias

[godplayer56]

Ok got it, so I'd be adding two more tests for it then :+1:

[godplayer56]

@bhearsum are the changes fine?

[bhearsum]

@godplayer56 - is this ready for another look? If so, please use the "Re-request review button" in the Reviewers list.

[godplayer56]

@godplayer56 - is this ready for another look? If so, please use the "Re-request review button" in the Reviewers list.

I am trying to resolve the post issues right now. I'll use it as soon as I am done resolving the issue :+1:

[godplayer56]

@bhearsum I have added assertions that confirm that the tests are failing for the right reason :+1: