Replace UUID with locator

[aruokhai]

Objective
Ensure locators are returned instead of uuid's under appointments section when user details are fetched through the cli using the getuser command.

Problem
locator is an appropriate identifier for appointments in the Bolt13 specification draft, which is also being used in the getappointments cli command , but in the getuser cli command uuid is being used which can be confusing . The solution is ensure getuser cli command makes use of locator and not uuid.

Changes

• Modified internal.rs get_user_info function to make use of locator and not uuid when returning user appointments info.
• Modified internal.rs get_user test to reflect the change from uuid to locator.

Scope Of Change
This is a non critical change, as it only affects the CLI.

closes #229

[sr-gi]

@aruokhai thanks for looking at this. This is going to make much more sense after merging #190, which gets rid of all in-memory data for the tower. Hence, a big refactor will be needed, but it would make the change way smaller. I'd suggest you wait until that is landed and take another look at it.

[sr-gi]

@aruokhai we just landed #190

[aruokhai]

would take a look at it again and start working on a new fix

[aruokhai]

added the necessary changes

[mariocynicys]

Other than the lint issue, LGTM!

You should squash the 3 commits into one and sign it. And another commit if you decide to modify get_appointments in the PR.

[sr-gi]

Oh wow, I have had a review for this pending for months and I never submitted it 🤦

Disregard the old comments.

[sr-gi]

Quick check, the concept is OK but the approach is not, check comments inline

[sr-gi]

Better, but I think the approach can still be improved

[mariocynicys]

Getting better. Need to filter trackers and some polishing.

[sr-gi]

Some additional comments on top of @mariocynicys's, looking better overall

[mariocynicys]

Awesome! Looking very good!

Needs some bigger/more generic tests though.

[sr-gi]

Looks pretty good. I left mostly rustlang improvements and some comments/nits.

The main things missing are more tests for get_appointment, this should be close to mergeable at this point

[mariocynicys]

Thanks! I think this is @ final stages.
Left some suggestions/refactors inline.

[mariocynicys]

Suggested change

	if locator_and_userid.is_some() {
	sql.push_str(" AND a.locator=(?1)");
	}
	// If a user_id is passed, filter even more.
	if locator_and_userid.is_some_and(|inner| inner.1.is_some()) {
	sql.push_str(" AND a.user_id=(?2)");
	}
	if let Some((locator, optional_userid)) = locator_and_userid {
	sql.push_str(" AND a.locator=(?1)");
	query_args.push(locator.to_vec());
	if let Some(user_id) = optional_userid {
	sql.push_str(" AND a.user_id=(?2)");
	query_args.push(user_id.to_vec());
	}
	}

[aruokhai]

I think this statement let mut stmt = self.connection.prepare(&sql).unwrap(); would prevent the addition of query_args.push(locator.to_vec()); . I believe only after the initialisation of the stmt variable can be query args be added , which also takes arrays as its parameters. Also the intial changes was made this way, in which i mean joined

if let Some((_, user_id)) = locator_and_userid {
            sql.push_str(" AND a.locator=(?1)");
            if user_id.is_some() {
                sql.push_str(" AND a.user_id=(?2)");
            }
        };

in which Sergio recommended i split to make it cleaner.

[mariocynicys]

Yup, this actually wouldn't work since stmt.query(...) won't accept a rust vector. I found this extension which allows preparing a statement with vector as args, but it's not worth the hustle. I only suggested this to not use the match below but it's not bad anyway.

Regarding nested checks, splitting the checks up in different lines makes sense when the method's args are Option<Locator> and Option<UserId>, because then we would add the appropriate filters without thinking about the dependency between them (we shouldn't nest the checks in this case).

But we went with the Option<(Locator, Option<UserId>)> route, in which there is a dependency between the locator and the user id (locator has to exist for user id to exist), so it makes more sense have the check for user id nested inside.

[mariocynicys]

Guess I found the review comment you are referring to. Well then, let's keep it as it is now until we all agree on how this should look.

@sr-gi What do you think about the comment above.

[mariocynicys]

Thanks!
Only one request from my side.
Looks good otherwise.

[mariocynicys]

Yup! thanks

[mariocynicys]

That's indeed way more readable, thanks!

[mariocynicys]

Thanks!
LGTM

[sr-gi]

Looks like this may be mergeable (?). Planing to give it a last look and merge if so. Needs rebase now that everything is green again