Accession edit feature #15
Conversation
blacksmith-welder
changed the title
thimios
requested review from
thimios,
balmas,
donaldjosephsmith and
brianzelip
blacksmith-welder
force-pushed
the
edit-accessions
branch
from
f57ac15
to
57abf99
Compare
Accession edit step definitions
blacksmith-welder
force-pushed
the
edit-accessions
branch
from
57abf99
to
40a31e7
Compare
| @@ -29,6 +29,13 @@ | |||
| Capybara::Selenium::Driver.new(app, browser: :firefox, options:) | |||
| end | |||
|
|
|||
| Capybara.register_driver :firefox_alternative_session do |app| | |||
There was a problem hiding this comment.
👍 didn't know you can do that!
| Given the Accession is opened in edit mode | ||
| When the user changes the 'Title' field | ||
| And the user clicks on 'Revert Changes' | ||
| Then the 'Title' field has the original value |
There was a problem hiding this comment.
see my comments on this test in the accession_edit step definitions
| end | ||
|
|
||
| When 'the user changes the {string} field' do |field| | ||
| fill_in field, with: "Updated Title #{SecureRandom.uuid}" |
There was a problem hiding this comment.
It looks like this was meant to be a generic When function that works with any field but the test is explicit to the Title. Probably should change the function name to match the behavior.
There was a problem hiding this comment.
Yes, That is correct. I missed that. It is generic now, and it's also moved inside shared/step_definitions.rb
| end | ||
|
|
||
| Then 'the {string} field has the original value' do |field| | ||
| expect(page).to have_field(field, with: "Accession Title #{@uuid}") |
There was a problem hiding this comment.
Same comment as previous - function name should match the behavior, which is specific to the Title field.
| end | ||
|
|
||
| Then 'all fields become editable' do | ||
| fill_in 'Identifier', with: 'Identifier' |
There was a problem hiding this comment.
this looks like it's just testing one field...
There was a problem hiding this comment.
After discussing this @kdimopulu, we decided that this step is unnecessary. We only need to check that we are on the edit page. The phrase fields become editable, implies that an input field is disabled and it is enabled after some action or condition. In the edit page, everything is editable. I have removed this.
| Then 'the Accession Date field has the original value' do | ||
| visit "#{STAFF_URL}/accessions/#{@accession_id}/edit" | ||
|
|
||
| expect(page).to have_field('Accession Date', with: '2000-01-01') |
There was a problem hiding this comment.
minor point but it might be better to have the original values as constants in case we decide to change them for some reason.
There was a problem hiding this comment.
Done.
However, I believe that maybe there should be a consistency between the original values, since some of them use strings interpolated with variables (uuids, or maybe something else), and it's not clear to the step definition writer when to use what.
The writer also checks the original values from the form that sees in the screen, so it is more straightforward to directly copy the strings, rather than having an extra lookup to another file, which may grow quite a lot in the future. In addition to that, in the future, we may have multiple records/factories, which may have multiple original values that may differ slightly, which again can cause confusion.
We can always refactor this this if it causes issues in the future.
There was a problem hiding this comment.
Ok I see that you added ORIGINAL_ACCESSION_DATE as a constant, so can you change line 91 to use it in the test?
| end | ||
|
|
||
| When 'User A changes the {string} field' do |field| | ||
| fill_in field, with: "Accession Title #{@uuid} updated" |
There was a problem hiding this comment.
Similar to previous comments, function definition should match the functionality which is title field specific not generic
There was a problem hiding this comment.
I have updated this to be more generic.
| end | ||
|
|
||
| When 'User B changes the {string} field' do |field| | ||
| @session.fill_in field, with: "Accession Title #{@uuid} updated" |
There was a problem hiding this comment.
Similar to previous comments, function definition should match the functionality which is title field specific not generic
There was a problem hiding this comment.
I have updated this to be more generic.
| end | ||
|
|
||
| Given 'the Accession is opened in edit mode by User B' do | ||
| @session = Capybara::Session.new(:firefox_alternative_session) |
There was a problem hiding this comment.
this is maybe nitpicking, but I think it would be clearer to name this variable something that reflects that it is an alternative or headless session
There was a problem hiding this comment.
Yes this makes sense. I wouldn't say it's nitpicking. It's good to have clarity and self-explanatory variable names in the code. This alternative session is defined here, and it may be headless or not, according to a command line option.
I have renamed this to @user_b_session, since all steps that make use of this session, refer to User B. Please let me know if you have a different name in mind.
No description provided.