Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accession suppress #26

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Accession suppress #26

wants to merge 3 commits into from

Conversation

kdimopulu
Copy link
Collaborator

No description provided.

@kdimopulu kdimopulu marked this pull request as draft November 15, 2024 10:57
Comment on lines +6 to +17
Scenario: Accession is suppressed
Given the Accession is not suppressed
When the user clicks on 'Suppress'
And the user clicks on 'Suppress' in the modal
Then the Accession now is suppressed
And the Accession cannot be accessed by archivists
Scenario: Accession is unsuppressed
Given the Accession is suppressed
When the user clicks on 'Unsuppress'
And the user clicks on 'Unsuppress' in the modal
Then the Accession now is not suppressed
And the Accession can be accessed by archivists
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The similarity in the naming of the Given and Then functions here made it so that I had to look really closely at the scenario implementation code to be sure there wasn't a bug because it looked like they should be doing the same thing.

What do you think about having the Given functions reference the action (i.e. suppress) and the Then functions reference the resulting state (i.e. available) ?

E.g.

Given the Accession can be suppressed
...
Then the Accession is not available

Given the Accession is suppressed
...
Then the Accession is available

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course @kdimopulu and @blacksmith-welder can respond here as well, but I'm not sure I'm following your concern @balmas. Which parts seem too similar? I think the "Accession is suppressed" and the "Accession is not suppressed" phrases match well and are appropriate for the Givens to focus on the current state of the record; "Accession can be suppressed" would kind of leave me wondering what factors contribute to that condition and have me inspecting the implementation code.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, I personally, would like to have exactly the same step names, with different implementations, that can be differentiated only by the Given, When, Then keywords.
When I was writing the step definitions, I came into the following conflict:

Given the Accession is suppressed
Then the Accession is not suppressed

and

Given the Accession is not suppressed
Then the Accession is suppressed

The Then the Accession is not suppressed causes conflict with Given the Accession is not suppressed (ambiguous match).
Unfortunately, Cucumber does not differentiate between Given, When, Then, and considers them the same.

That is why I used the word now for Then steps, to distinguish between them, which I don't like since it's not intuitive and redundant.

I do agree that it is a bit confusing when you look at the step definitions, but the reasoning here is that we are focusing on the Gherkin description of the feature, and we do not care about the step definitions. Since the step definitions are always shared, and scattered all over the place, the only way to actually follow what the feature does is within the *.feature files.

Generally, the bigger picture is to make the Gherkin steps as simple, intuitive, and straightforward as possible, so that even an untrained user can write them, without having to look at any of the step definitions, or any documentation (ideal scenario). It would be nice if the user could infer steps by looking at a subset of existing step names.

I do agree with Donald, that can be suppressed and is available are quite ambiguous, and may imply behavior that is not described here.

Maybe, we can write more detailed steps in the future for suppressed records, which describe more accurately what available means.

From the docs:

To suppress an accession record
A suppressed record will remain in the database, but it can be hidden from most staff users and unavailable to most functions, including searching, browsing, or reporting. The functionality can be useful to retain records for accessions that have been completely deaccessioned from the repository, or to restrict access to unfinished records or to records of confidential material. Once the record is suppressed, only authorized users will be able to find it in the system. If you suppress a published record, the suppression will take priority over the publish function All suppressed records can be unsuppressed.

However, this is my personal viewpoint, and in the end, it is a matter of preference, and it is also subject to discussion.

Let me know what you think @balmas. Do you want me to apply these changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants