Made URL of new channel creation page more generic #4863

shruti862 · 2024-12-21T19:04:46Z

Summary

Made URL of new channel creation page more generic to avoid 404 channel not found message on refreshing the page
Screencast from 22-12-24 12:22:47 AM IST.webm

References

Fixes issue #2389

Reviewer guidance

…

bjester

Thanks for your contribution! I tested it and it works, but I have several requested changes and suggestions

bjester · 2025-01-21T15:16:55Z

contentcuration/contentcuration/frontend/shared/views/channel/ChannelModal.vue

+            .then(channel => {
+              if (this.isNew) {
+                const newChannelId = channel.id;
+                window.location = window.Urls.channel(newChannelId);


I realize this line of code was existing, but ideally it should have used window.location.assign. Since the behavior of the new channel creation page is changing, I think it might be better to either use window.location.replace directly, or perhaps replace the current location with the RouteNames.CHANNEL_EDIT route using the new channel ID, then assign to window.Urls.channel(newChannelId)

bjester · 2025-01-21T15:17:27Z

contentcuration/contentcuration/frontend/shared/views/channel/ChannelModal.vue

+              if (this.isNew) {
+                const newChannelId = channel.id;
+                window.location = window.Urls.channel(newChannelId);
+                this.$store.dispatch('showSnackbarSimple', this.$tr('channelCreated'));


Since the page needs a reload, I don't know that this snackbar is worth it. The display of it must race the page load, which could mean it's never shown, depending on various factors related to speed.

bjester · 2025-01-21T15:17:39Z

contentcuration/contentcuration/frontend/shared/vuex/channel/actions.js

    if (!id) {
-      throw ReferenceError('id must be defined to update a channel');
+      throw new ReferenceError('id must be defined to update a channel');


bjester · 2025-01-21T15:18:34Z

contentcuration/contentcuration/frontend/shared/vuex/channel/actions.js

+    }
+
+    // Log the data before sending it to create a new channel
+    console.log('Creating new channel with data:', newChannelData);


Please remove the console.log statements in this vuex action. Thank you

bjester · 2025-01-21T15:37:41Z

contentcuration/contentcuration/frontend/shared/views/channel/ChannelModal.vue

+
+      // If there's no channelId (i.e., it's a new channel), skip verifications
+      if (!channelId) {
+        //this.isNew = true;


Please remove this commented out code.

bjester · 2025-01-21T15:39:47Z

contentcuration/contentcuration/frontend/shared/views/channel/ChannelModal.vue

+      // If there's no channelId (i.e., it's a new channel), skip verifications
+      if (!channelId) {
+        //this.isNew = true;
+        this.header = 'New Channel'; // Default header for a new channel


This seems redundant with the code in mounted. I suggest removing this.

bjester · 2025-01-21T15:40:16Z

contentcuration/contentcuration/frontend/shared/views/channel/ChannelModal.vue

-      this.header = this.channel.name; // Get channel name when user enters modal
+      if (this.isNew) {
+        // For a new channel, set a default header
+        this.header = 'New Channel';


This needs to use our UI strings. For example, using this.$tr with the preexisting string key in the $trs object of the component.

bjester · 2025-01-21T15:45:09Z

contentcuration/contentcuration/frontend/shared/vuex/channel/actions.js

+    return Channel.createModel(newChannelData).then(response => {
+      console.log('API Response:', response);
+
+      const createdChannel = response.data || response; // Adjust based on API structure


It must be one or the other-- either response.data or response? I suggest making this specific.

bjester · 2025-01-21T15:47:08Z

contentcuration/contentcuration/frontend/shared/vuex/channel/actions.js

+    }
+    if (contentDefaults !== NOVALUE) {
+      newChannelData.content_defaults = contentDefaults;
+    }


The above code is very similar to the if branch above in this function. I'm requesting deduplication of the code because the only real difference should be the presence of id which isn't significant enough to warrant the duplication. For example, it seems there are differences here in how it handles the thumbnail above, which leads to a regression that I can see when you upload a thumbnail for a new channel and adjust it's cropping.

shruti862 · 2025-01-23T10:44:27Z

hey @bjester ,I’ve implemented the changes you suggested, and everything is working as expected in terms of the channel creation process. However, I’ve encountered another issue. Whenever I close the ChannelModal without creating a new channel or when selecting the "EXIT WITHOUT SAVING" option after editing, an empty channel is still being created which on reloading the empty channel vanishes.

I don't know why this is happening ,Could you kindly assist in identifying the root cause of this behavior and help me resolve it?

MisRob · 2025-01-27T13:48:01Z

However, I’ve encountered another issue. Whenever I close the ChannelModal without creating a new channel or when selecting the "EXIT WITHOUT SAVING" option after editing, an empty channel is still being created which on reloading the page vanishes.

@shruti862 was this the case before your latest changes in this pull request? If so, feel free to open a new issue if there is not one yet. If your changes introduce it, it'd be best to spend some time debugging and resolve it in this pull request.

shruti862 · 2025-01-29T09:47:46Z

@MisRob, the empty channel creation issue is due to my recent changes. After debugging, I have identified the root cause and will resolve it soon.

shruti862 · 2025-01-29T11:45:23Z

Hey, @MisRob @bjester , createChannel function was creating an empty channel ::

  newChannel() {
        this.$analytics.trackClick('channel_list', 'Create channel');
        this.createChannel().then(id => {
          this.$router.push({
            name: RouteNames.CHANNEL_EDIT,
            params: { channelId: id, tab: 'edit' },
            query: { last: this.$route.name },
          });
        });
      },

When I removed the function from the line of code:

newChannel() {
        this.$analytics.trackClick('channel_list', 'Create channel');
        
          this.$router.push({
            name: RouteNames.NEW_CHANNEL,
            query: { last: this.$route.name },
          });
      },

Due to this change ,one frontend test is failing which says new channel should be created when new channel button is clicked which should not be the case because clicking on create button in new channel modal should create a new channel.

shruti862 · 2025-01-31T14:16:14Z

Hey @MisRob @bjester,

Could you assist me with the failing frontend test issue? I’d like to understand the cause and how to resolve it. Your guidance would be greatly appreciated.

Thanks!

MisRob · 2025-01-31T14:26:57Z

Hello @shruti862, we will follow-up with you, however please be patient as we have a large queue of pull request to work through. It's good to send a reminder if we don't reply roughly after a week. Thank you!

shruti862 · 2025-01-31T16:36:33Z

@MisRob ,I apologize if I caused any inconvenience. I appreciate your time and will wait patiently. Thank you!

MisRob · 2025-01-31T17:23:17Z

No problem @shruti862 at all. Thank you.

MisRob · 2025-02-03T14:36:50Z

Hi @shruti862

the empty channel creation issue is due to my recent changes. After debugging, I have identified the root cause and will resolve it soon.

Good news, thanks.

Due to this change ,one frontend test is failing which says new channel should be created when new channel button is clicked which should not be the case because clicking on create button in new channel modal should create a new channel

This sounds that you just need to update tests to align them well with your changes, is that right? Basically the test suite would ideally correspond to user experience so you're welcome to update the test accordingly.

MisRob · 2025-02-03T15:19:36Z

@shruti862 Also if you find any other tests related to this feature that are perhaps still passing but their description or implementation doesn't correspond anymore, that's always great to update too. Thanks for reaching out to clarify this and let us know if you needed anything else.

shruti862 · 2025-02-04T07:18:51Z

Hey @MisRob ,Thank you for clarifying my doubt :). I will soon update the test.

shruti862 · 2025-02-04T11:04:12Z

Hey @MisRob @bjester, I’ve implemented all the suggested changes and also fixed the failing frontend test. Whenever you have a moment, could you please take a look at my changes and let me know if any further improvements are needed? :)

shruti862 and others added 3 commits December 22, 2024 00:27

made url of new channel page generic

d3eb1ba

Merge branch 'learningequality:unstable' into new-branch

41805ec

[pre-commit.ci lite] apply automatic fixes

35d9f72

bjester linked an issue Jan 13, 2025 that may be closed by this pull request

Refreshing the channel creation page leads to 404 channel not found #2389

Open

MisRob requested review from bjester and akolson January 16, 2025 15:34

MisRob added the TODO: needs review label Jan 16, 2025

shruti862 and others added 6 commits January 19, 2025 13:14

Merge branch 'learningequality:unstable' into new-branch

e3d7acb

new change

0c9816e

new commit

c57cdb9

Merge branch 'new-branch' of github.com:shruti862/studio into new-branch

73b58e2

new commit

fb97997

[pre-commit.ci lite] apply automatic fixes

b946b5a

bjester requested changes Jan 21, 2025

View reviewed changes

shruti862 and others added 4 commits January 23, 2025 15:52

suggested changes done

f618cc2

Merge branch 'new-branch' of github.com:shruti862/studio into new-branch

00e1f54

new commit

b6e3714

[pre-commit.ci lite] apply automatic fixes

e1dce41

new change

20a1104

shruti862 and others added 2 commits January 29, 2025 17:21

Merge branch 'new-branch' of github.com:shruti862/studio into new-branch

e0f6859

[pre-commit.ci lite] apply automatic fixes

8537b03

shruti862 requested a review from bjester January 30, 2025 11:44

shruti862 and others added 3 commits February 4, 2025 16:28

modified channelList.spec.js test

302ded7

Merge branch 'new-branch' of github.com:shruti862/studio into new-branch

94329d7

[pre-commit.ci lite] apply automatic fixes

35fa0f6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Made URL of new channel creation page more generic #4863

Made URL of new channel creation page more generic #4863

shruti862 commented Dec 21, 2024

bjester left a comment

bjester Jan 21, 2025

bjester Jan 21, 2025

bjester Jan 21, 2025

bjester Jan 21, 2025

bjester Jan 21, 2025

bjester Jan 21, 2025

bjester Jan 21, 2025

bjester Jan 21, 2025

bjester Jan 21, 2025

shruti862 commented Jan 23, 2025 •

edited

Loading

MisRob commented Jan 27, 2025

shruti862 commented Jan 29, 2025

shruti862 commented Jan 29, 2025

shruti862 commented Jan 31, 2025

MisRob commented Jan 31, 2025

shruti862 commented Jan 31, 2025

MisRob commented Jan 31, 2025

MisRob commented Feb 3, 2025

MisRob commented Feb 3, 2025

shruti862 commented Feb 4, 2025

shruti862 commented Feb 4, 2025

Made URL of new channel creation page more generic #4863

Are you sure you want to change the base?

Made URL of new channel creation page more generic #4863

Conversation

shruti862 commented Dec 21, 2024

Summary

References

Reviewer guidance

bjester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shruti862 commented Jan 23, 2025 • edited Loading

MisRob commented Jan 27, 2025

shruti862 commented Jan 29, 2025

shruti862 commented Jan 29, 2025

shruti862 commented Jan 31, 2025

MisRob commented Jan 31, 2025

shruti862 commented Jan 31, 2025

MisRob commented Jan 31, 2025

MisRob commented Feb 3, 2025

MisRob commented Feb 3, 2025

shruti862 commented Feb 4, 2025

shruti862 commented Feb 4, 2025

shruti862 commented Jan 23, 2025 •

edited

Loading