When we have a story which can pass but produces a defect to be fixed, the PO or BA involved will usually write a new story and immediately insert it into the sprint on the basis that it is important enough because it is a business priority.
Is this proper for scrum? If not, what should be done?
When I work with teams I help them to differentiate between "a task to complete a story" and a "defect".
If the team has not reached the Definition of Done, then there is no defect. The story is just "not done". If they find an issue, before reaching Done, I coach teams to just create another task (sub-task in Jira) for the story. Until all your tasks are complete, the story isn't done. It puts the focus on completion, not on tracking defects.
If, after a story has reached Done and has been demoed, a defect is found, then it's a new Product Backlog Increment. The product owner decides it's priority and asks the team to take it into a sprint through the normal planning process.
According to the Scrum Guide, "only the Development Team can change its Sprint Backlog during a Sprint". This means that it is wrong for anyone other than a member of the Development Team to add an item to the Sprint Backlog. The Product Owner should be adding items to the Product Backlog, prioritizing them, and working with the Development Team to refine them so they can be worked on.
However, I see a broader issue. If you are working on a Product Backlog Item and, in the course of testing the work, find that the work is not complete, it may be necessary to fix the defect to ensure that there is a Potentially Releasable Increment at the completion of the Sprint. The Development Team should work with the Product Owner to figure out if the issue would prevent the Increment from being released. You may also need to consider other process requirements before simply fixing the issue.
What you're describing isn't technically Scrum; it violates several key Scrum and agile principles. If your Scrum-like process allows for changes in scope or level of effort within an iteration in ways that routinely impact the Sprint Goal, then you will have to reduce your forecast for each Sprint to account for the defects and uncertainty introduced by your process.
You appear to be conflating bugs/defects with new work, because the following is an agile oxymoron (emphasis mine):
When we have a story which can pass but produces a defect to be fixed, the PO or BA involved will usually write a new story and immediately insert it into the sprint on the basis that it is important enough because it is a business priority.
By definition, you can't have a story that meets a reasonable Definition of Done (DoD) but is also "defective." Depending on the DoD agreed upon between the Development Team and the Product Owner, a story could possibly be "Done" while generating technical debt or iterative improvements that need to be addressed in a future Sprint, but cannot result in an unreleasable increment because of serious bugs, defects, or regressions.
In other words, if the issues identified are serious enough that they must be resolved before the increment can be released, the increment can't be considered "done." On the other hand, if the issues don't prevent the increment from being considered as releasable by the Development Team or the Product Owner (or according to the agreed-upon Definition of Done), then the issue is new work that must be placed onto the Product Backlog and scheduled for a future iteration.
The team should undertake the following action items as part of the framework's ongoing inspect-and-adapt cycle:
Ensure that each Sprint has a cohesive Sprint Goal that ties the work together with a clear scope and a focused objective.
Ensure that only the Development Team is able to add work directly to the Sprint Backlog.
Ensure that any reviews performed by the business analysts or Product Owner as part of the acceptance criteria for stories are:
Create a project definition that distinguishes clearly between bugs/defects and new work in order to prevent scope creep.
Reframe your Sprint process as a way to plan and scope work in cycles, as opposed to treating is as a way to task the team with work.
What all of these items have in common is that they work together to ensure that each Sprint is treated as a time box in which the team can develop a potentially-releasable increment. Because Scrum is an iterative development methodology, the underlying principle is that all work must be scoped for, planned, and delivered in tightly-bound time boxes. Any practice that puts Sprint Goals in jeopardy or that doesn't respect the time box is an anti-pattern, and should be dealt with in the team's Sprint Retrospectives and other inspect-and-adapt ceremonies.
