Code review comment for lp:~elopio/u1-test-utils/page-objects

Revision history for this message
Natalia Bidart (nataliabidart) wrote :

Branch looks good! Approving with some comments:

* I personally would not recommend changing the order of the param to existing API (so I would not swap is_site_recognized with user, I would leave user first). But I also understand this lib is not heavily used, so you can leave as is. What I do think we need to leave in place is a default value for is_site_recognized.

* I advice against having method that return different stuff depending on the branch being used. Any chance that
create_new_account returns something more unified (I'm not sure what type does site_not_recognized.yes_sign_me_in() has)? In any case, if you feel like this is the right approach, feel free to leave as is.

* Same two comments for sign_in.

review: Approve

« Back to merge proposal