Skip to content

Comments

Object v2 channel contract tests mc#248

Merged
marcin-cebo merged 11 commits intomasterfrom
ObjectV2_Channel_contractTests_mc
Oct 27, 2022
Merged

Object v2 channel contract tests mc#248
marcin-cebo merged 11 commits intomasterfrom
ObjectV2_Channel_contractTests_mc

Conversation

@marcin-cebo
Copy link
Contributor

No description provided.

public class Include implements ParameterEnricher {
static final String INCLUDE_PARAM_NAME = "include";
static final String INCLUDE_CUSTOM_PARAM_VALUE = "custom";
static final String INCLUDE_CUSTOM_PARAM_VALUE = "custom,type,status";
Copy link
Contributor

@kleewho kleewho Oct 13, 2022

Choose a reason for hiding this comment

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

Shouldn't this be handled by ObjectApiEndpoint?

Copy link
Contributor Author

@marcin-cebo marcin-cebo Oct 14, 2022

Choose a reason for hiding this comment

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

The thing is that method Include#enrichParameters is executed after ObjectApiEndpoint#createBaseParams. So even you set proper values in ObjectApiEndpoint#createBaseParams then they might be overritten in Include#enrichParameters.
I could revert the change you pointed out but then modification of Include#enrichParameters is required from

from

    public Map<String, String> enrichParameters(Map<String, String> baseParams) {
        final Map<String, String> enrichedMap = new HashMap<>(baseParams);
        if (!inclusionFlags.isEmpty()) {
            enrichedMap.put(INCLUDE_PARAM_NAME, join(inclusionFlags));
        }
        return enrichedMap;
    }

to

    public Map<String, String> enrichParameters(Map<String, String> baseParams) {
        final Map<String, String> enrichedMap = new HashMap<>(baseParams);
        if (!inclusionFlags.isEmpty()) {
            String currentIncludeValue = enrichedMap.get(INCLUDE_PARAM_NAME);
            inclusionFlags.add(currentIncludeValue);
            enrichedMap.put(INCLUDE_PARAM_NAME, join(inclusionFlags));
        }
        return enrichedMap;
    }

Do you see other options?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe remove change in ObjectApiEndpoint and that one here and put something like this:

static final Collection<String> DEFAULT_INCLUDE_FLAGS = Arrays.asList("type", "status");

private final List<String> inclusionFlags = new ArrayList<>(DEFAULT_INCLUDE_FLAGS);

That way you should have it regardless of custom being added or not. The question that I have for you though. Are you sure we should add always both type and status? I'm pretty sure it's a status only for memberships, but somehow I don't see it in Kotlin implementation so I don't know anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed on a cal I am working on this.

@Given("the data for {string} member")
fun the_data_for_member(memberName: String) {
val channelMember: PNMembers = loadMember(memberName)
channelMembersState.uuids = listOf(PNUUID.uuid(channelMember.uuid.id))
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why populate this uuids list here. Nothing in the step says that it should be done. I'd implemented it by just storing the member data in the state class. Later steps should decide what to do with it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. I will change it.

setChannelMetadataState.pnChannelMetadata?.name = pnChannelMetadata.name
setChannelMetadataState.pnChannelMetadata?.description = pnChannelMetadata.description
setChannelMetadataState.pnChannelMetadata?.status = pnChannelMetadata.status
setChannelMetadataState.pnChannelMetadata?.type = pnChannelMetadata.type
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not like this?

setChannelMetadataState.pnChannelMetadata = pnChannelMetadata

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I will modify it.

@marcin-cebo marcin-cebo merged commit d4eebfd into master Oct 27, 2022
@marcin-cebo marcin-cebo deleted the ObjectV2_Channel_contractTests_mc branch October 27, 2022 14:39
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.

2 participants