One unrelated-but-included-intentions on entities patch as 189985ef2fc was changing this, but there is still some little things to do.

CommentFileSizeAuthor
#17 0001-task-890216-follow-up-by-marvil07-sdboyer-Fix-accoun.patch10.47 KBmarvil07
#15 890216-15-missed-some-field-refs.patch1.64 KBsdboyer
#13 890216-vcs-username-v2.patch3.87 KBmarvil07
#12 890216-vcs-username.patch1.83 KBsdboyer
#1 0001-change-nomeclature-to-use-s-instead-of-s.patch4.26 KBmarvil07
0001-change-nomeclature-to-use-s-instead-of-s.patch4.6 KBmarvil07

Comments

marvil07’s picture

marvil07 commented
StatusFileSize
new 0001-change-nomeclature-to-use-s-instead-of-s.patch4.26 KB

get out that var_dump :-p

marvil07’s picture

marvil07 commented
Issue tags: +git phase 2

tagging

chrisstrahl’s picture

chrisstrahl commented
Issue tags: +git sprint 2

tagging for sprint 2

chrisstrahl’s picture

chrisstrahl commented
Assigned: Unassigned » sdboyer
sdboyer’s picture

sdboyer commented
Title: change nomeclature to use $vcs_username{,s} instead of $username{,s} » change nomeclature to use $username(s) instead of $vcs_username(s)
Status: Needs review » Fixed

Think I've got this all done locally, will be reintegrating into CVS presently. Also fixing the title, it was actually describing the opposite of what needed to be done.

sdboyer’s picture

sdboyer commented
Status: Fixed » Needs review

Not so sure about this, actually.

sdboyer’s picture

sdboyer commented
Status: Needs review » Closed (won't fix)

Actually, it's more helpful to leave this with a separate property name by default. Given that the allowed characters are different for most vcs usernames vs. Drupal usernames, we may as well as use a different variable name for it. So, scratching this.

marvil07’s picture

marvil07 commented
Status: Closed (won't fix) » Active

Ok, so this is only about consistency.
So we need to revert http://github.com/sdboyer/versioncontrol/commit/189985ef2fc592c15be7700b...

sdboyer’s picture

sdboyer commented
Issue tags: +git sprint 3

OK, wow I COMPLETELY forgot about how this actually was a problem because of the db <-> class property naming mapping. It's not just a convenience thing like I presented last week, this actually needs to be done. So, this is indeed active, but because it needs to be finished up, not reverted.

sdboyer’s picture

sdboyer commented

Actually, yeah, let's revert that, and change the name of the field in the database. Whew.

sdboyer’s picture

sdboyer commented
Status: Active » Needs work

Reverted the changes listed in the original commit. Will post a patch presently with the db changes.

sdboyer’s picture

sdboyer commented
StatusFileSize
new 890216-vcs-username.patch1.83 KB

Sorry, I got a bit distracted with trying to test this change. Ended up hacking on tests, then ended up writing half of that abstracted framework for allowing tests in VCAPI to test backends, per their availability. It's kinda cool, I'll post a patch in another issue soon.

Anyway, the patch is attached here. It seems to basically work well, but another set of eyes would be great - especially until we get the tests definitively fixed.

marvil07’s picture

marvil07 commented
Status: Needs work » Needs review
StatusFileSize
new 890216-vcs-username-v2.patch3.87 KB

I think it was only one more, but I also change one doc blocks and a variable name to be consistent.

sdboyer’s picture

sdboyer commented
Status: Needs review » Fixed

Committed: http://drupal.org/cvs?commit=446862

sdboyer’s picture

sdboyer commented
Assigned: sdboyer » marvil07
StatusFileSize
new 890216-15-missed-some-field-refs.patch1.64 KB

OK, noticed that I missed a couple of fields to change over. And when I fixed them, there were suddenly a lot MORE failures in the versioncontrol_account_status tests...which is weird. Why were they passing before, when this was clearly broken?

marvil07, could you check out the attached patch and look into fixing the tests (and make sure all the necessary changes have been made wrt the original purpose of this issue)?

sdboyer’s picture

sdboyer commented
Status: Fixed » Needs work
marvil07’s picture

marvil07 commented
Priority: Minor » Normal
Status: Needs work » Needs review
StatusFileSize
new 0001-task-890216-follow-up-by-marvil07-sdboyer-Fix-accoun.patch10.47 KB

It really took me a while to figure out why account status is failing at the test, that was the only automated way I have to actually review this nomenclature change is working. But I end up fixing several stuff at its functionality.

So, I have a patch that make account status test work! (aka all test passing!)

    1. Fix the last username/vcs_username nomenclature problem
    
       - patch by sam at #890216-15
       - Just another s/username/vcs_username at
         versioncontrol_account_edit_form_submit().
    
    2. Handle special two-PK entinty controller for account
    
       - Deprecate returning FALSE on versioncontrol_user_accounts_load().
       - Manually get the right repo object at account load.
       - Use only one approach for specially keyed array returned at
         VersioncontrolAccountController::executeQuery(): nested array.
       - Process the nested array mentioned at
         VersioncontrolAccountController::buildEntities() to get one-level
         array that can be processed by VersioncontrolEntityController
         caching mechanism.
       - Some minor refactor at versioncontrol_user_accounts_title_callback()
         and versioncontrol_account_page().

@sdboyer: I would like to commit this after your review.

marvil07’s picture

marvil07 commented
Issue tags: +git sprint 4

tagging

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
commented
Status: Needs review » Reviewed & tested by the community

This looks good. I'd say it's okay to be committed.

marvil07’s picture

marvil07 commented
Status: Reviewed & tested by the community » Fixed

I just committed the last patch, feel free to re-open this if necessary.

\o/ patches working 100%

Status: Fixed » Closed (fixed)
Issue tags: -git phase 2, -git sprint 2, -git sprint 3, -git sprint 4

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue tags: +git phase 2, +git sprint 2, +git sprint 3, +git sprint 4

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.

  • Commit 2ba8f80 on repository-families, drush-vc-sync-unlock by sdboyer: 
    Per issue #890216, partial reversion of changes to property naming in...
  • Commit f7307be on repository-families, drush-vc-sync-unlock by sdboyer:
    Issue #890216: change nomenclature to use $username(s) instead of $...
  • Commit 2ac50ad on repository-families, drush-vc-sync-unlock by marvil07:
    task #890216 follow-up by marvil07, sdboyer: Fix account status...

  • Commit 2ba8f80 on repository-families by sdboyer: 
    Per issue #890216, partial reversion of changes to property naming in...
  • Commit f7307be on repository-families by sdboyer:
    Issue #890216: change nomenclature to use $username(s) instead of $...
  • Commit 2ac50ad on repository-families by marvil07:
    task #890216 follow-up by marvil07, sdboyer: Fix account status...