Fix part of #17712: Acceptance tests for logged in learner

AFZL210 commented 3 weeks ago

Overview

This PR fixes part of #17712 .
This PR does the following: Acceptance test for logged-in learner. learner can subscribe to a creator and view all explorations authored by that creator.

Essential Checklist

Please follow the instructions for making a code change.

[x] I have linked the issue that this PR fixes in the "Development" section of the sidebar.
[x] I have checked the "Files Changed" tab and confirmed that the changes are what I want to make.
[x] I have written tests for my code.
[x] The PR title starts with "Fix #bugnum: " or "Fix part of #bugnum: ...", followed by a short, clear summary of the changes.
[x] I have assigned the correct reviewers to this PR (or will leave a comment with the phrase "@{{reviewer_username}} PTAL" if I can't assign them directly).

Proof that changes are correct

https://github.com/oppia/oppia/actions/runs/8813042490/job/24191195669?pr=20195

Desktop

Mobile

PR Pointers

Never force push! If you do, your PR will be closed.
To reply to reviewers, follow these instructions: https://github.com/oppia/oppia/wiki/Make-a-pull-request#step-5-address-review-comments-until-all-reviewers-approve
Some e2e tests are flaky, and can fail for reasons unrelated to your PR. We are working on fixing this, but in the meantime, if you need to restart the tests, please check the "If your build fails" wiki page.
See the Code Owner's wiki page for what code owners will expect.

oppiabot[bot] commented 3 weeks ago

Hi @AFZL210, can you complete the following:

The proof that changes are correct has not been provided, please make sure to upload a image/video showing that the changes are correct. Or include a sentence saying "No proof of changes needed because" and the reason why proof of changes cannot be provided. Thanks!

oppiabot[bot] commented 3 weeks ago

Assigning @DubeySandeep for the first pass review of this PR. Thanks!

oppiabot[bot] commented 2 weeks ago

Assigning @DubeySandeep for code owner reviews. Thanks!

AFZL210 commented 2 weeks ago

@rahat2134 The thing is, multiple tests require exploration with just end exploration, just as a placeholder. It would be better if we could have a function that creates an exploration and publishes, rather than writing all the code in beforeAll in all other tests. and createExplorationWithOnlyEndInteraction function does just that it. we will just need to call one function instead of doing each step in beforeAll().

rahat2134 commented 2 weeks ago

@rahat2134 The thing is, multiple tests require exploration with just end exploration, just as a placeholder. It would be better if we could have a function that creates an exploration and publishes, rather than writing all the code in beforeAll in all other tests. and createExplorationWithOnlyEndInteraction function does just that it. we will just need to call one function instead of doing each step in beforeAll().

The purpose of making composite functions like createExplorationWithMinimumContent and publishExplorationWithContent was to do the same thing that you are suggesting else we have seperate method for each step.

We can have this composite function also. We can have a better method name for this. For example: createAndPublishAMinimalExplorationWithTitle(); Thanks.

/cc @seanlip PTAL.

seanlip commented 2 weeks ago

@AFZL210 @rahat2134 I can't understand what is going on in this discussion, it seems to have gotten detached from what the original concern is and I'm not sure which line of the PR it refers to.

What is the point of issue and what exactly are you asking me for input on? Thanks.

rahat2134 commented 2 weeks ago

@seanlip Actually There is a composite function used by @AFZL210 which is ->

  /**
   * Function for creating an exploration with only EndExploration interaction with given title.
   */
  async createExplorationWithOnlyEndInteraction(
    title: string
  ): Promise<string | null> {
    await this.navigateToCreatorDashboardPage();
    await this.navigateToExplorationEditorPage();
    await this.dismissWelcomeModal();
    await this.createExplorationWithMinimumContent(
      'Exploration intro text',
      'End Exploration'
    );
    await this.navigateToSettingsTab();
    await this.updateTitleTo(title);
    await this.updateGoalTo('OppiaAcceptanceTestsCheck');
    await this.selectCategory('Algebra');
    await this.selectLanguage('Arabic');
    await this.addTags(['TagA', 'TagB', 'TagC']);
    await this.saveExplorationDraft();
    return await this.publishExploration();
  }

Which is basically used to create and publish an exploration with minimal content or we can say with end Interaction.

We already have createExplorationWithMinimumContent and publishExplorationWithContent. Now Afzal has created another more composite function by which we can do both things like create an exploration and publish exploration. Is it OK to have this function? If yes then we can have a better name likecreateAndPublishAMinimalExplorationWithTitle();

seanlip commented 2 weeks ago

Thanks for explaining, @rahat2134.

I think this actually goes back to the general question of API design. In general we don't want to have two functions that do more or less the same thing. However it's fine to have a function that combines two others if we usually treat that as a single step. That said, to reduce code duplication, it would be better for that function to reuse the existing functions rather than specifying a completely alternative implementation path.

So, I think the main thing I would consider here is: if we want to introduce this new function, why isn't it sufficient to use the existing functions (or augment them -- e.g. in terms of functionality or allowing them to be customized slightly), to the point where a new function is needed altogether (as well as the additional maintenance that keeping those functions in sync would entail)? I think this needs to be justified because it looks like the current implementation doesn't make use of the already-implemented functions.

@AFZL210 @rahat2134 any thoughts on this (and can you propose what should be done based on the principles outlined above)? Thanks!

AFZL210 commented 2 weeks ago

Thank you, @seanlip. I believe we can keep this new function, as it does not add any new logic, it just transfers control to existing functions. I believe it's useful, as we can skip manually calling functions for steps like going to the creator dashboard, opening the editor page, etc.

cc/ @rahat2134

seanlip commented 2 weeks ago

@AFZL210 That doesn't fully address the concern. Does the logic of this function duplicate other functions? You need to justify why using existing functions will not suffice.

AFZL210 commented 2 weeks ago

@AFZL210 That doesn't fully address the concern. Does the logic of this function duplicate other functions? You need to justify why using existing functions will not suffice.

We are using existing functions and creating a new function to call those existing functions so we don't need to call 5-6 functions in beforeAll everytime we need an exploration.

oppiabot[bot] commented 1 week ago

Unassigning @rahat2134 since they have already approved the PR.

AFZL210 commented 1 week ago

@DubeySandeep PTAL. Thanks!

oppiabot[bot] commented 1 week ago

Unassigning @DubeySandeep since they have already approved the PR.

oppiabot[bot] commented 1 week ago

Hi @AFZL210, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks!

oppia / oppia